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