[libvirt] [PATCH] Bind to ip_addr support.
Hi, Mainly because I think Daniel always gives good feedback and comments, plus I don't want to be a troll on this list, I decided the time had come to spend some minutes to hack up a patch :) It has been tested: tcp 0 0 192.168.0.5:16509 *:* LISTEN vs tcp 0 0 *:16509 *:* LISTEN Only lefts me: By making a contribution to this project, I, Stefan de Konink, certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it; and (d) In the case of each of (a), (b), or (c), I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license indicated in the file. Signed-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl> Yours Sincerely, Stefan de Konink
On Sat, May 10, 2008 at 05:31:53PM +0200, Stefan de Konink wrote:
Hi,
Mainly because I think Daniel always gives good feedback and comments, plus I don't want to be a troll on this list, I decided the time had come to spend some minutes to hack up a patch :)
It has been tested:
tcp 0 0 192.168.0.5:16509 *:* LISTEN
vs
tcp 0 0 *:16509 *:* LISTEN
The code all looks good to me, but can you also provide an update for the default configuration file in qemud/libvirtd.conf with a (commented out) example setting. Regards, Daniel. -- |: 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 :|
On Sat, 10 May 2008, Daniel P. Berrange wrote:
The code all looks good to me, but can you also provide an update for the default configuration file in qemud/libvirtd.conf with a (commented out) example setting.
Oops :) That didn't make it in the patch. But was already written. (Please fix my poor English) Stefan
On Sat, May 10, 2008 at 06:43:11PM +0200, Stefan de Konink wrote:
On Sat, 10 May 2008, Daniel P. Berrange wrote:
The code all looks good to me, but can you also provide an update for the default configuration file in qemud/libvirtd.conf with a (commented out) example setting.
Oops :) That didn't make it in the patch. But was already written. (Please fix my poor English)
THanks, I have applied this patch to CVS now. The only change I made was to rename the config parameter from 'ip_addr' to 'listen_addr', since we're likely to have other IP addrs in the future... Regards, Daniel. -- |: 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 :|
On Wed, 14 May 2008, Daniel P. Berrange wrote:
On Sat, May 10, 2008 at 06:43:11PM +0200, Stefan de Konink wrote:
On Sat, 10 May 2008, Daniel P. Berrange wrote:
The code all looks good to me, but can you also provide an update for the default configuration file in qemud/libvirtd.conf with a (commented out) example setting.
Oops :) That didn't make it in the patch. But was already written. (Please fix my poor English)
THanks, I have applied this patch to CVS now. The only change I made was to rename the config parameter from 'ip_addr' to 'listen_addr', since we're likely to have other IP addrs in the future...
I see the ip_addr in libvirtd.conf needs updating. Stefan
On Thu, May 15, 2008 at 02:59:52PM +0200, Stefan de Konink wrote:
On Wed, 14 May 2008, Daniel P. Berrange wrote:
On Sat, May 10, 2008 at 06:43:11PM +0200, Stefan de Konink wrote:
On Sat, 10 May 2008, Daniel P. Berrange wrote:
The code all looks good to me, but can you also provide an update for the default configuration file in qemud/libvirtd.conf with a (commented out) example setting.
Oops :) That didn't make it in the patch. But was already written. (Please fix my poor English)
THanks, I have applied this patch to CVS now. The only change I made was to rename the config parameter from 'ip_addr' to 'listen_addr', since we're likely to have other IP addrs in the future...
I see the ip_addr in libvirtd.conf needs updating.
Opps, yes, done that now 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 :|
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. 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. Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl> Stefan
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 :|
On Tue, 13 May 2008, Daniel P. Berrange wrote:
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.
I have have send a patch to avahi to prevent this. In avahi you are now able to set the actual interface that is allowed to be broadcasted on. allow-interfaces and deny-interfaces.
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.
Oki.
+ + 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.
Update and resubmit? Stefan
On Tue, 13 May 2008, Stefan de Konink wrote:
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.
** RETRACT PATCH** Patch should work, but obviously doesn't. (Or actually it hides the service too well). Stefan
Stefan de Konink schreef:
On Tue, 13 May 2008, Stefan de Konink wrote:
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.
** RETRACT PATCH**
Patch should work, but obviously doesn't. (Or actually it hides the service too well).
To elaborate: 1) Avahi doesn't eat IPs as host. What is strange. 2) With a valid hostname something like xen01.local the service is still exported as two addresses, what this patch wanted to prevent. Asked on the avahi mailinglist what their ideas on these points were. Stefan
participants (2)
-
Daniel P. Berrange -
Stefan de Konink