Re: [Libvir] PATCH: Avahi advertisement for libvirtd

On Tue, Sep 18, 2007 at 04:13:46AM -0400, Daniel Veillard wrote: > On Tue, Sep 18, 2007 at 03:50:18AM +0100, Daniel P. Berrange wrote: > > This is a serious patch at supporting Avahi advertisement of the libvirtd > > service. > > > > - configure by default will probe to see if avahi is available and if > > found will enable appropriate code. > > > > --with-avahi will force error if not found > > --without-avahi will disable it with no checking > > > > - HAVE_AVAHI is defined in config.h if avahi is found & used to conditionally > > enable some code in qemud/qemud.c > > > > - HAVE_AVAHI is also a Makefile conditional to enable compilation of the > > mdns.c and mdns.h files. A little makefile rearrangement was needed to > > make sure variables like EXTRA_DIST were defined before we appended to > > them with += > > > > - The code in mdns.c contains all the support for dealing with the Avahi > > APIs. > > > > - The primary Avahi API is horribily complicated for day-to-day > > use in application code, exposing far too much of the event loop and > > state machine. So we expose a simplified API to libvirt in mdns.h > > Heh, did you tell the Avahi devels ? No, but perhaps I should. I don;t know if glib has a higher level API for accessing the Avahi stuff. I know there's a out-of-box glib event loop impl for it. > > - The Avahi client library is basically a shim which talks to the Avahi > > daemon using DBus system daemon. The DBus stuff doesn't leak out of > > the Avahi APIs - it is loosely couple - all we need do is provide Avahi > > with an event loop implementation which was surprisingly easy. The > > libvirtd daemon does now indirectly link with DBus, but I don't see any > > problem with this. Don't want it - then use --without-avahi > > That's fine, as long as the extensions don't decrease portability I > don't see this as a problem. Ok I tweaked the configure.in checks I did for avahi slightly. Now, if you don't have 'pkg-config' available, it'll simply disable the avahi feature instead of failing the entire script. It'll auto-detect by default, and can be overriden with the --with/--without-avahi flags. > > - We advertise a service name of _libvirt._tcp The IETF draft recommends > > use of the name from /etc/services associated with your app. There is a > > way to register official Avahi services names. We don't have an /etc/service > > name registered either though. > > I rememember we looked at the IANA stuff for registering a port number > http://www.redhat.com/archives/libvir-list/2007-June/msg00204.html > I suggested using > "10 " What SHORT name (14 CHARACTER MAXIMUM) do you want associated with this port number?" > > libvirt" > > But we never did that registration step. Ok, well our mDNS name matches. So guess we should look at actually doing a application to register both the mDNS & service name. > > - This patch does not advertise any per-VM VNC server instances, but I have > > prepared the APIs in mdns.h to be ready to support that with minimal effort. > > A pre-requisite for this is an extension to the driver API to get async > > signals when VMs start & stop, since making the daemon poll hypervisors > > will suck big time. > > > > When implemented each VM will be its own mdns 'group' and the VNC server > > associated with it will be the 'service' advertised in that group. > > > > Having applied this patch & started the daemon, if /etc/init.d/avahi-daemon > > is running, you should see the service advertised on the LAN. As mentioned > > earlier if you start Avahi daemon after libvirt it should detect this too. > > Sounds excellent ! I've now tested it on Fedora 7, 8 and RHEL-5 & its working very nicely. > > These are all mildy abusing mdns / zeroconf, but then x509 certificates don't > > really fit into the model of 'zero conf' in the first place. If people want > > true zero-conf then the (SSH) tunnel is better (and always available), but > > if they've setup certificates they should still be allowed to use zero-conf > > to at least locate hosts. So mildly abusing the rules is reasonable IMHO. > > Maybe suggesting that application developpers default to SSH when using > a server autodetected with Avahi is the most practical ATM assuming we don't > find a way to advertise the FQDN. Unless we can find the domain from the > locally installed certificate, after all if people want to use the certificate > they should have some installed locally and then we can probably guess the > right domain name, no ? Yeah, I reckon recommending SSH is a good general rule for this. I'll keep thinking about a better way to handle x509 validation by the client - we can easily add more TXT records to the mDNS stuff increemntally if needed. > > +dnl Avahi library > > +AC_ARG_WITH(avahi, > > + [ --with-avahi use avahi to advertise remote daemon], > > + [], > > + [with_avahi=check]) > > + > > +if test "$with_avahi" = "check"; then > > + PKG_CHECK_EXISTS(avahi-client >= $AVAHI_REQUIRED, [with_avahi=yes], [with_avahi=no]) > > +fi > > + > > +if test "$with_avahi" = "yes"; then > > + PKG_CHECK_MODULES(AVAHI, avahi-client >= $AVAHI_REQUIRED) > > + AC_DEFINE_UNQUOTED(HAVE_AVAHI, 1, [whether Avahi is used to broadcast server presense]) > > +else > > + AVAHI_CFLAGS= > > + AVAHI_LIBS= > > +fi > > +AM_CONDITIONAL(HAVE_AVAHI, [test "$with_avahi" = "yes"]) > > +AC_SUBST(AVAHI_CFLAGS) > > +AC_SUBST(AVAHI_LIBS) > > I assume that if an OS has Avahi, then it has pkg-check, in that case it > really should not be a problem. As mentioned above, I also tweaked it to disable Avahi if pkg-config is missing completely. > > + > > +if HAVE_AVAHI > > +libvirtd_SOURCES += mdns.c mdns.h > > +libvirtd_CFLAGS += $(AVAHI_CFLAGS) > > +libvirtd_LDADD += $(AVAHI_LIBS) > > +else > > +EXTRA_DIST += mdns.c mdns.h > > +endif > > Wouldn't adding them to EXTRA_DIST in any case be good enough ? > if SOURCES and EXTRA_DIST carry the same is that really a problem when > building the archive ? Yep, I made this change - adding them to EXTRA_DIST unconditionally. > Patch is surprizingly small, looks good, I would commit to CVS, no pending > issue looks like a potential real problem for users, and more testing would > be good. Ok, its committed! Also added the BuildRequires to the spec file & docs on the two new config file options.