[libvirt] [PATCHv3 1/3] util: capabilities detection for dnsmasq

[Eric Blake]

> 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]

> +++ 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.

> @@ -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.

> @@ -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?

> +++ 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).  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?

> +
> +    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;

> +fail:
> +    p = strchr(buf, '\n');
> +    if (!p)
> +        p = strchr(buf, '\0');

Slightly more efficient as:

p = strchrnul(buf, '\n')

> +
> +/** 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.

> +++ 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.