[libvirt] [PATCH] Bind to ip_addr support.

[Daniel P. Berrange]

On Sat, May 10, 2008 at 07:42:51PM +0200, Stefan de Konink wrote:
> About mdns, do you think it would be a good thing to *not* follow the
> Avahi advise and explicitly set the host the service is running on? In my
> humble opinion I think that would be a wise decision.
> 
> host 	The host this services is residing on. We recommend to pass NULL
> here, the daemon will than automatically insert the local host name in
> that case
> 
> I wonder how avahi will decide if the service doesn't run on 0.0.0.0 but
> on a specific address. Then we still have domain left, but with a bit
> smart implementation upstream that could just work.

Well the  'host' parameter of the avahi_entry_group_add_service() method
does not control which interfaces Avahi broadcasts on. It merely controls
the hostname included in the advertisement. So if a machine had 2 nics and
libvirt was only listening on one NIC, by setting this explicitly it means
we'd be broadcasting on both NICs still, but the advertisment will have an
IP address that's not reachable via one of the NICs. 

So the client seeing the advertisment and trying to use it, would likely
get a 'host unreachable' message (due to be unable to find the IP address
listed), instead of a 'connection refused' message (due to being able to
reach the IP, but libvirt not listening).

So unless I'm mis-understanding what this parameter does, I'm not convinced
that this change makes sense.

> The attached patch explicitly sets the host of the mDNS advertisement.
> 
> 
> Because of some 'unexpected' behavior... I hoped strdup(NULL) would just
> work, but it didn't, I placed it inside an if loop. The debug message
> doesn't mind '(null)' but I think 0.0.0.0 is more appropriate.

NB, libvirt is explicitly written to be protocol independant - so avoid
refering to '0.0.0.0' which is IPv4 specific. We fully support IPv6 and
it is enabled by default if the host OS has IPv6 enabled (which all 
Fedora since FC6 do)

> @@ -440,18 +441,28 @@
>      }
>  }
>  
> -struct libvirtd_mdns_entry *libvirtd_mdns_add_entry(struct libvirtd_mdns_group *group, const char *type, int port) {
> +struct libvirtd_mdns_entry *libvirtd_mdns_add_entry(struct libvirtd_mdns_group *group, const char *type, const char *host, int port) {
>      struct libvirtd_mdns_entry *entry = malloc(sizeof(*entry));
>  
> -    AVAHI_DEBUG("Adding entry %s %d to group %s", type, port, group->name);
> +    AVAHI_DEBUG("Adding entry %s %s %d to group %s", type, (host == NULL ? "0.0.0.0" : host), port, group->name);

This is misleading - NULL indicates bind to all addresses which includes
IPv6 if enabled - 0.0.0.0 is IPv4 only. Since this is only a debug message
its not too troubling. Just pass in 'host' unchanged - sprintf will output
it as '(null)' which is more accurate.

> +    
> +    if (host != NULL) {
> +        if (!(entry->host = strdup(host))) {
> +	    free(entry->type);
> +            free(entry);

There is indentation whitespace damage here. Please check the HACKING
file for details of how to setup vim and emacs to comply with libvirt
indentation rules automatically.

Regards,
Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|