[libvirt] [PATCH] Drop empty argument from dnsmasq call
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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.
participants (3)
-
Eric Blake -
Guido Günther -
Laine Stump