[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: DNS based service discovery
From:       Jakub Stachowski <stachowski () hypair ! net>
Date:       2004-10-28 19:13:29
Message-ID: 200410282113.29702.stachowski () hypair ! net
[Download RAW message or body]

Dnia środa, 27 października 2004 21:55, Thiago Macieira napisał:
> Jakub Stachowski wrote:
> >Hello,
> >
> >For the last month I have been working on DNS based service discovery
> >(ZeroConf, Rendezvous) for KDE. You can get it from kdenonbeta/kdnssd.
>
> Problems with the code, as it stands now:
> - it's licensed GPL, so it can't be included in kdelibs. It has to be LGPL
> to be in kdelibs -- except for the ioslave and kcm.

Done - copied and pasted from kdelibs.

> - classes have no d-pointers or virtual hooks

These d-pointers can be useful for better abstracting RemoteService and 
QueryBase - instead of derived classes for mDNS and uDNS they can be made 
into *Private part.

> - classes have inlined methods and member variables -- might not be a good
> idea

Inlined methods removed, by what is wrong with member variables? Most classes 
in kdelibs have them.

> - use of C-style casts. Please replace with static_cast<> or
> reinterpret_cast<> where appropriate.

Done.

>
> Also, please check that your conversion to QStrings in
> responder.cpp:resolve_reply are using the correct encoding (it should
> probably be UTF-8). In the same code, are you sure the 255- and 256-byte 
> buffers will never overflow?

Yes, because ( from howl's text_record.h):
#define SW_TEXT_RECORD_MAX_LEN 255
sw_result HOWL_API  sw_text_record_iterator_next(	sw_text_record_iterator 
self, char key[SW_TEXT_RECORD_MAX_LEN], sw_uint8 val[SW_TEXT_RECORD_MAX_LEN],
sw_uint32	*val_len);

But no doubt it is better to use SW_TEXT_RECORD_MAX_LEN instead of hardcoded 
255 in case of future changes.

>
> Please don't define AllServices (servicebrowser.h) as a static const
> QString variable. Add it to a class and move the definition to the .cpp
> file, or make it global and move to the .cpp (don't forget the 'extern').
>
OK, moved to class;

> In ServiceBrowser::startBrowse(), can you cache the result of
> m_domains->domains() and its end iterator? Same thing for
> UDNSQuery::havePtr().
>
> Actually, for UDNSQuery, I'd rather you did not use QDns at all. Please use
> KResolver for PTR-resolution, if that's what you need. 

How can I do it? I see only API for straight name-to-address resolution, not 
name-to-name/

> At the very least, 
> in havePtr(), cache the QStringList and use iterators, instead of
> QStringList::at(), which can be O(n).
>
> ServiceList is descended from QPtrList. Please make it a QValueList, since
> the QPtr* are going away in Qt4 anyways.

So I should use QValueList<ServiceBase*> ? Well, that requires reimplementing 
find() because I'm interested in matching services, not  matching pointers.
>
> This is a personal taste, but don't you use indentation or break long
> lines? Your code is somewhat hard to read...

It seems I'm spoiled by KDevelop too much :-)

>
> Aside from that, libhowl seem to be yet another great example of code that
> should have been written in C++.
>
> >- good and simple API for service publishing and discovery
>
> Seems to be achieving that. However, is it really necessary to make the
> resolutions go "_http._tcp"?

I have to thank Matthias Ettrich and slides from his aKademy talk for that. 
As second part of service type can be only either _tcp or _udp I can make it 
enum.

>
> I'd suggest using two parameters and automatically prepending the
> underscore.
>
> >- compatibility with other implementations - MacOSX, GNOME 2.8, Firefox
> > plugin (under development). This is why I choose write module for mDNS
> > support instead of SLP (and trying to revive knot) - because it seems to
> > be quite widespread not only in operating systems and desktop
> > environments but also in hardware like network printers and some wireless
> > routers.
>
> By using SRV-based resolution? I'd agree.
>
> >- support for unicast DNS - for discovering services in larger networks
> > and over internet
>
> I also want to provide SRV-based resolution for normal stuff, including web
> browsing.
>
> >- ioslave - for easy use from file selector, konqueror and sidebar. For
> > now it can find ftp and http servers.
>
> Probably integration with network:/ (lisa) is desireable.

They are quite different - DNS-SD provides list of services regardless of 
their physical location (so you can reconnect printer named "Photo printer" 
to another machine and it will still work fine) when lisa provides list of 
computers on network and list of services provided by them. 
Possible way of integration: add new category "Network hosts" (or similar) to 
dnssd:// (in addition to typical like "FTP servers").  URL belonging to this 
category would be internally (in dnssd slave) redirected to lisa ioslave - 
exactly as media:// does. I could use ForwardingSlaveBase from kioslave/media 
for that.

>
> >- my goal is to have kdnssd included in KDE - what do you think about it?
> >"useful enough to be bugfixed and included" , " try again in several
> > months" or "it sucks, just drop it" ? And if answer is "include", do you
> > think it can be done in KDE 3.4?
>
> Aside from what I pointed out above, I think we could give it a try.
> However, as things go, you'll first need an application using it for it to
> move away from kdenonbeta. Then you'll need a second for it to go into 
> kdelibs.

Ehm, what "second" ? Second application using it?

>
> Also, can it work at all without libhowl? How difficult would it be to
> reimplement it, if necessary?

I could try to reimplement it using parts from mdnsd 
(http://dotlocal.org/mdnsd/) . They are only 2 important .c files so it would 
be possible to embed it. I can't say anything about difficulty and required 
time before I try doing it.

> >Requirements:
> >- libhowl 0.9.6 . Unfortunately it crashes, patch was sent to author (also
> >available in kdnssd/patches). He responded that new version of libhowl is
> >going to be released in a week.
> >- Qt 3.3 - there is bug in QDns  TXT record handling - it was reported and
> >will be fixed in next version. Patch is also available. Good news is that
> >this bug in not too serious and it probably can be worked around.
>
> That's two requirements I would rather not have (an extra lib required in
> kdelibs and QDns).

I understand that extra lib is problem but what is wrong with QDns? 
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

[prev in list] [next in list] [prev in thread] [next in thread]