> 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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list