8:37 a.m.
Hi,
Libvirt currently fails to start with dnsmasq >= 2.56.
since dnsmasq now bails out with empty arguments. See
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=613944
for the Debian bug and
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=589885
for the upstream reasoning.
Tested with 2.55 and 2.56. O.k. to apply?
Cheers,
-- Guido
12:32 p.m.
On 02/18/2011 07:37 AM, Guido Günther wrote:
Hi,
Libvirt currently fails to start with dnsmasq >= 2.56.
since dnsmasq now bails out with empty arguments. See
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=613944
for the Debian bug and
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=589885
for the upstream reasoning.
Tested with 2.55 and 2.56. O.k. to apply?
Cheers,
-- Guido
I was worried that this might cause a regression with older dnsmasq,
given the previous conversation on this topic:
https://www.redhat.com/archives/libvir-list/2010-December/msg00504.html
What's the oldest dnsmasq that libvirt needs to support? I'm guessing
dnsmasq 2.45, used in RHEL 5.x, is still relevant. A quick[1] look
through that source code:
./src/option.c: {"pid-file", 2, 0, 'x'},
shows that --pid-file has always been an optional_argument[2], which means:
--pid-file
--pid-file=
-x
specify no pid file,
--pid-file=xyz
-xxyz
specify xyz as the pid file, and
--pid-file ''
-x ''
have always parsed the '' argument in isolation, and just been ignoring
it until this recent change.
[1] Would have been quicker if I could have found an online browseable
git repository, instead of having to download the tarball from
http://www.thekelleys.org.uk/dnsmasq/ - but thank heavens for open
source guaranteeing that I can at least find the source!
[2] Would it hurt dnsmasq to use symbolic constants from <getopt.h>
instead of magic numbers?
Therefore, ACK to this patch.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:37 p.m.
On 02/18/2011 11:32 AM, Eric Blake wrote:
Oops - I sent my initial reply without finishing reading your patch.
./src/option.c: {"pid-file", 2, 0, 'x'},
But your patch was about conf-file. Thankfully:
./src/option.c: {"conf-file", 2, 0, 'C'},
so the same arguments apply.
--pid-file
--pid-file=
-x
specify no pid file,
Actually, with getopt_long(), --pid-file and -x set the optional
argument to NULL, while --pid-file= sets the optional argument to the
empty string. Depending on the rest of the program, that slight
difference could have an impact, so we'd better stick with the same form
of --pid-file=.
Therefore, ACK to this patch.
Actually, I have a nit; could you please fix it first?
+++ b/src/network/bridge_driver.c
@@ -477,7 +477,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
virCommandAddArgPair(cmd, "--pid-file", pidfile);
/* *no* conf file */
- virCommandAddArgList(cmd, "--conf-file=", "", NULL);
+ virCommandAddArgList(cmd, "--conf-file=", NULL);
This might as well be the shorter (and faster):
virCommandAddArg(cmd, "--conf-file=");
now that it is only one argument.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:52 p.m.
On 02/18/2011 01:49 PM, Guido Günther wrote:
On Fri, Feb 18, 2011 at 11:37:53AM -0700, Eric Blake wrote:
[..snip..]
> This might as well be the shorter (and faster):
>
> virCommandAddArg(cmd, "--conf-file=");
>
> now that it is only one argument.
Like in the attached patch?
ACK; please push!
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:24 p.m.
On Fri, Feb 18, 2011 at 01:52:03PM -0700, Eric Blake wrote:
On 02/18/2011 01:49 PM, Guido Günther wrote:
> On Fri, Feb 18, 2011 at 11:37:53AM -0700, Eric Blake wrote:
> [..snip..]
>> This might as well be the shorter (and faster):
>>
>> virCommandAddArg(cmd, "--conf-file=");
>>
>> now that it is only one argument.
> Like in the attached patch?
ACK; please push!
Pushed now. Thanks,
-- Guido
1 p.m.
On 02/18/2011 01:32 PM, Eric Blake wrote:
On 02/18/2011 07:37 AM, Guido Günther wrote:
> Hi,
> Libvirt currently fails to start with dnsmasq>= 2.56.
> since dnsmasq now bails out with empty arguments. See
>
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=613944
>
> for the Debian bug and
>
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=589885
>
> for the upstream reasoning.
>
> Tested with 2.55 and 2.56. O.k. to apply?
> Cheers,
> -- Guido
I was worried that this might cause a regression with older dnsmasq,
given the previous conversation on this topic:
https://www.redhat.com/archives/libvir-list/2010-December/msg00504.html
I had a similar email composed, including the same reference, but was
going about investigating RHEL5 compatibility in a different way -
building libvirt with this change on RHEL5 and seeing what happens (what
I should have done when I first encountered it, rather than sweeping it
under the rug). Since my RHEL5 installation was in a state of disrepair,
you beat me to the punch :-)
Basically my thinking is similar to yours, though. RHEL5's dnsmasq is
probably the oldest that we need to worry about supporting, so if having
the empty arg there breaks something new, and removing it doesn't break
RHEL5's dnsmasq, then I'm happy to see it go.
5026
days inactive
5026
days old
6 comments
3 participants
participants (3)
-
Eric Blake -
Guido Günther -
Laine Stump