2:09 p.m.
On 11/28/2012 06:13 PM, Eric Blake wrote:
> In order to optionally take advantage of new features in dnsmasq
when
> the host's version of dnsmasq supports them, but still be able to run
> on hosts that don't support the new features, we need to be able to
> detect the version of dnsmasq running on the host, and possibly
> determine from the help output what options are in this dnsmasq.
>
> networkxml2argvtest.c creates 2 "artificial" dnsmasqCaps objects at
> startup - one "restricted" (doesn't support --bind-dynamic) and one
> "full" (does support --bind-dynamic). Some of the test cases use one
> and some the other, to make sure both code pathes are tested.
Should we also follow the lead of tests/qemuhelpdata/ and do an actual
capture from released dnsmasq binaries, to ensure that our parser for
those files matches our artificial dnsmasqCaps objects? More on this
below...[1]
I had thought about that when making this patch, but wanted to keep
things as simple as possible, since I'll probably have to backport it
all the way to 0.8.7 :-/ Because of that, I'd like to keep things as
they are for this patch, and expand it later.
> +++ b/src/libvirt_private.syms
> @@ -246,13 +246,19 @@ virDevicePCIAddressParseXML;
> dnsmasqSave;
>
> -
> # domain_audit.h
Spurious whitespace change? Then again, we haven't been very
consistent on one vs. two blank lines between .h files.
Fixed.
> @@ -891,7 +899,8 @@ cleanup:
> }
>
> static int
> -networkStartDhcpDaemon(virNetworkObjPtr network)
> +networkStartDhcpDaemon(struct network_driver *driver,
Given Dan's recent rename of 'struct qemud_driver *driver' to
the friendlier 'virQEMUDriverPtr driver', should we do a similar
change here? But if so, separate it to its own patch.
Yes, but as you say, in a separate patch.
> @@ -935,7 +944,9 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
> if (dctx == NULL)
> goto cleanup;
>
> - ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile,
> dctx);
> + dnsmasqCapsRefresh(&driver->dnsmasqCaps, false);
> +
> + ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile,
> dctx, driver->dnsmasqCaps);
Long line; worth wrapping?
Fixed.
> +++ b/src/network/bridge_driver.h
> @@ -1,7 +1,7 @@
> /*
> * network_driver.h: core driver methods for managing networks
> *
> - * Copyright (C) 2006, 2007, 2011 Red Hat, Inc.
> + * Copyright (C) 2006-2012 Red Hat, Inc.
You asked me about this one on IRC. If libvirt had a single
copyright holder, then I'd say this is okay (it follows the
convention[2] given by the FSF, where if a repository is publicly
available, then touching any one file in that repository counts
as a copyrightable claim on the package as a whole, and that
all files in the package may claim the same range of copyright
years as the package as a whole, even for files that were not
touched in a particular year). It is a bit more questionable
here (since we have NOT required anyone to assign copyright,
libvirt has a multitude of copyright holders), but I still
think you can get away with this change (Red Hat does indeed
own the copyrights on a vast majority of the code base, and
has made copyrightable contributions in every single year in
this range).
That last bit is the important part - the change is actually correct
since (as I confirmed by looking through the git history) this file (or
one of its predecessors, as it was moved/split a couple times) *was*
modified by someone from Red Hat at least once in every year in that range.
I guess it all boils down to how likely we are
to ever try and defend the copyright in court (FSF has a much
easier job of that task, thanks to them requiring copyright
assignment; whereas with libvirt, it is up to individual
copyright holders over their individual portions of the
overall work to decide what they would defend in court).
[2] https://www.gnu.org/prep/maintain/maintain.html#Copyright-Notices
"To update the list of year numbers, add each year in which you have made nontrivial
changes to the package. (Here we assume you’re using a publicly accessible revision
control server, so that every revision installed is also immediately and automatically
published.) When you add the new year, it is not required to keep track of which files
have seen significant changes in the new year and which have not. It is recommended and
simpler to add the new year to all files in the package, and be done with it for the rest
of the year."
"You can use a range (‘2008-2010’) instead of listing individual years (‘2008, 2009,
2010’) if and only if: 1) every year in the range, inclusive, really is a “copyrightable”
year that would be listed individually; and 2) you make an explicit statement in a README
file about this usage."
> +
> +#define SKIP_BLANKS(p) do { while ((*(p) == ' ') || (*(p) == '\t'))
> (p)++; } while (0)
Could we use virSkipSpaces() instead of open-coding this?
I was just copying exactly what's already in qemu_capabilities.c,
without even looking for alternatives.
Looking at virSkipSpaces, it would work correctly in its current form.
Prior to 0.9.4 (when you fixed it), it would also skip backslashes,
which isn't technically correct for us, but would still coincidentally
work (since dnsmasq doesn't have any backslashes in its version string),
so I can safely change it to use virSkipSpaces.
> +
> + p = buf;
> + if (!STRPREFIX(p, DNSMASQ_VERSION_STR))
> + goto fail;
> + p += strlen(DNSMASQ_VERSION_STR);
Slightly more compact as:
p = STRSKIP(buf, DNSMASQ_VERSION_STR);
if (!p)
goto fail;
Sure. Again, caused by just copying from qemu_capabilities.c.
> +fail:
> + p = strchr(buf, '\n');
> + if (!p)
> + p = strchr(buf, '\0');
Slightly more efficient as:
p = strchrnul(buf, '\n')
Likewise.
> +
> +/** dnsmasqCapsRefresh:
> + *
> + * Refresh an existing caps object if the binary has changed. If
> + * there isn't yet a caps object (if it's NULL), create a new one.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +static dnsmasqCapsPtr
> +dnsmasqCapsNewEmpty(const char *binaryPath)
Comment attached to the wrong function.
Oops. I had decided at the end that function wasn't self-explanatory
enough and went back in to add the comment. I guess I wasn't paying
close attention to where I was :-P
> +++ b/tests/networkxml2argvtest.c
> @@ -140,23 +148,34 @@ static int
> mymain(void)
> {
> int ret = 0;
> + dnsmasqCapsPtr restricted
> + = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.48", DNSMASQ);
> + dnsmasqCapsPtr full
> + = dnsmasqCapsNewFromBuffer("Dnsmasq version
> 2.63\n--bind-dynamic", DNSMASQ);
[1] Here is where I'm worried that we might want to also test sample
actual output, in separate data files (which are also tagged as immune
to syntax checking). But that would be a separate test (much as
qemuhelptest is separate from qemuxml2argvtest), and could therefore
be delayed to a later patch, so I'm okay with the amount of testing
done in this particular test.
I had some points you might want to clean up, but they are all minor
(no logic bugs, just style or micro-optimizations), so I'm okay
giving ACK. I've seen enough patches from you that I trust you to
make the cleanups and/or post followups without needing to see a
v4 just to address my findings.
Thanks! I'm pushing with the fixes you indicated.