10:31 a.m.
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(a)kinkrsoftware.nl>
Yours Sincerely,
Stefan de Konink
11:38 a.m.
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 :|
11:43 a.m.
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
3:58 p.m.
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 :|
7:59 a.m.
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
8:19 a.m.
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 :|
12:42 p.m.
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(a)kinkrsoftware.nl>
Stefan
6:47 p.m.
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 :|
12:21 a.m.
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
6:34 a.m.
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
6:54 a.m.
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
6035
days inactive
6040
days old
11 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Stefan de Konink