On Nov 21, 2012, at 8:55 PM, Laine Stump <laine@laine.org> 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.

This patch implements a greatly simplified version of the capabilities code we already have for qemu. A dnsmasqCaps device can be created and populated either from running a program on disk, reading a file with the concatenated output of "dnsmasq --version; dnsmasq --help", or examining a buffer in memory that contains the concatenated output of those two commands. Simple functions to retrieve capabilities flags, the version number, and the path of the binary are also included.

bridge_driver.c creates a single dnsmasqCaps object at driver startup, and disposes of it at driver shutdown. Any time it must be used, the dnsmasqCapsRefresh method is called - it checks the mtime of the binary, and re-runs the checks if the binary has changed.

I would try it myself but I'm on vacation with only an iPad. What happens if no dnsmasq is installed? It doesn't impede libvirtd starting up if no networks are configured?

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. --- src/libvirt_private.syms | 8 +- src/network/bridge_driver.c | 59 ++++++---- src/network/bridge_driver.h | 7 +- src/util/dnsmasq.c | 260 ++++++++++++++++++++++++++++++++++++++++++++ src/util/dnsmasq.h | 20 +++- tests/networkxml2argvtest.c | 57 ++++++---- 6 files changed, 366 insertions(+), 45 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a07139..24d2033 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -244,13 +244,19 @@ virDevicePCIAddressParseXML; # dnsmasq.h dnsmasqAddDhcpHost; dnsmasqAddHost; +dnsmasqCapsGet; +dnsmasqCapsGetBinaryPath; +dnsmasqCapsGetVersion; +dnsmasqCapsNewFromBuffer; +dnsmasqCapsNewFromFile; +dnsmasqCapsNewFromBinary; +dnsmasqCapsRefresh; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; dnsmasqReload; dnsmasqSave;

- # domain_audit.h virDomainAuditCgroup; virDomainAuditCgroupMajor; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c153d36..34923ea 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -85,6 +85,7 @@ struct network_driver { char *networkConfigDir; char *networkAutostartDir; char *logDir; + dnsmasqCapsPtr dnsmasqCaps; };

@@ -271,7 +272,8 @@ networkFindActiveConfigs(struct network_driver *driver) { char *radvdpidbase;

ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, obj->def->name, - &obj->dnsmasqPid, DNSMASQ)); + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));

if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) { virReportOOMError(); @@ -389,6 +391,9 @@ networkStartup(int privileged) { goto out_of_memory; }

+ if (!(driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ))) { + /* attempt to continue anyway */ + }

if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, @@ -514,6 +519,8 @@ networkShutdown(void) { if (driverState->iptables) iptablesContextFree(driverState->iptables);

+ virObjectUnref(driverState->dnsmasqCaps); + networkDriverUnlock(driverState); virMutexDestroy(&driverState->lock);

@@ -616,7 +623,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virNetworkIpDefPtr ipdef, const char *pidfile, virCommandPtr cmd, - dnsmasqContext *dctx) + dnsmasqContext *dctx, + dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { int r, ret = -1; int nbleases = 0; @@ -848,7 +856,8 @@ cleanup:

int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, - char *pidfile, dnsmasqContext *dctx) + char *pidfile, dnsmasqContext *dctx, + dnsmasqCapsPtr caps) { virCommandPtr cmd = NULL; int ret = -1, ii; @@ -876,8 +885,8 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;

- cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) { + cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); + if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx, caps) < 0) { goto cleanup; }

@@ -891,7 +900,8 @@ cleanup: }

static int -networkStartDhcpDaemon(virNetworkObjPtr network) +networkStartDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { virCommandPtr cmd = NULL; char *pidfile = NULL; @@ -935,7 +945,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); if (ret < 0) goto cleanup;

@@ -988,7 +1000,8 @@ cleanup: * Returns 0 on success, -1 on failure. */ static int -networkRefreshDhcpDaemon(virNetworkObjPtr network) +networkRefreshDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { int ret = -1, ii; virNetworkIpDefPtr ipdef; @@ -996,7 +1009,7 @@ networkRefreshDhcpDaemon(virNetworkObjPtr network)

/* if there's no running dnsmasq, just start it */ if (network->dnsmasqPid <= 0 || (kill(network->dnsmasqPid, 0) < 0)) - return networkStartDhcpDaemon(network); + return networkStartDhcpDaemon(driver, network);

/* Look for first IPv4 address that has dhcp defined. */ /* We support dhcp config on 1 IPv4 interface only. */ @@ -1038,7 +1051,8 @@ cleanup: * Returns 0 on success, -1 on failure. */ static int -networkRestartDhcpDaemon(virNetworkObjPtr network) +networkRestartDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { /* if there is a running dnsmasq, kill it */ if (network->dnsmasqPid > 0) { @@ -1047,7 +1061,7 @@ networkRestartDhcpDaemon(virNetworkObjPtr network) network->dnsmasqPid = -1; } /* now start dnsmasq if it should be started */ - return networkStartDhcpDaemon(network); + return networkStartDhcpDaemon(driver, network); }

static int @@ -1245,7 +1259,8 @@ cleanup: }

static int -networkRefreshRadvd(virNetworkObjPtr network) +networkRefreshRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network) { /* if there's no running radvd, just start it */ if (network->radvdPid <= 0 || (kill(network->radvdPid, 0) < 0)) @@ -1265,7 +1280,8 @@ networkRefreshRadvd(virNetworkObjPtr network) #if 0 /* currently unused, so it causes a build error unless we #if it out */ static int -networkRestartRadvd(virNetworkObjPtr network) +networkRestartRadvd(struct network_driver *driver, + virNetworkObjPtr network) { char *radvdpidbase;

@@ -1313,8 +1329,8 @@ networkRefreshDaemons(struct network_driver *driver) * dnsmasq and/or radvd, or restart them if they've * disappeared. */ - networkRefreshDhcpDaemon(network); - networkRefreshRadvd(network); + networkRefreshDhcpDaemon(driver, network); + networkRefreshRadvd(driver, network); } virNetworkObjUnlock(network); } @@ -2224,7 +2240,8 @@ networkStartNetworkVirtual(struct network_driver *driver,

/* start dnsmasq if there are any IP addresses (v4 or v6) */ - if ((v4present || v6present) && networkStartDhcpDaemon(network) < 0) + if ((v4present || v6present) && + networkStartDhcpDaemon(driver, network) < 0) goto err3;

/* start radvd if there are any ipv6 addresses */ @@ -2988,7 +3005,7 @@ networkUpdate(virNetworkPtr net, /* these sections all change things on the dnsmasq commandline, * so we need to kill and restart dnsmasq. */ - if (networkRestartDhcpDaemon(network) < 0) + if (networkRestartDhcpDaemon(driver,network) < 0) goto cleanup;

} else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST) { @@ -3009,8 +3026,8 @@ networkUpdate(virNetworkPtr net, }

if ((newDhcpActive != oldDhcpActive && - networkRestartDhcpDaemon(network) < 0) || - networkRefreshDhcpDaemon(network) < 0) { + networkRestartDhcpDaemon(driver, network) < 0) || + networkRefreshDhcpDaemon(driver, network) < 0) { goto cleanup; }

@@ -3021,7 +3038,7 @@ networkUpdate(virNetworkPtr net, * can just update the config files and send SIGHUP to * dnsmasq. */ - if (networkRefreshDhcpDaemon(network) < 0) + if (networkRefreshDhcpDaemon(driver, network) < 0) goto cleanup;

} @@ -3030,7 +3047,7 @@ networkUpdate(virNetworkPtr net, /* only a change in IP addresses will affect radvd, and all of radvd's * config is stored in the conf file which will be re-read with a SIGHUP. */ - if (networkRefreshRadvd(network) < 0) + if (networkRefreshRadvd(driver, network) < 0) goto cleanup; }

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 0fae275..3af74a1 100644 --- a/src/network/bridge_driver.h +++ 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. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -48,7 +48,8 @@ int networkGetNetworkAddress(const char *netname, char **netaddr)

int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, - dnsmasqContext *dctx) + dnsmasqContext *dctx, + dnsmasqCapsPtr caps) ; # else /* Define no-op replacements that don't drag in any link dependencies. */ @@ -56,7 +57,7 @@ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx, caps) 0 # endif

typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index 9d1c07b..03b3792 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -39,8 +39,10 @@

#include "internal.h" #include "datatypes.h" +#include "bitmap.h" #include "dnsmasq.h" #include "util.h" +#include "command.h" #include "memory.h" #include "virterror_internal.h" #include "logging.h" @@ -583,3 +585,261 @@ dnsmasqReload(pid_t pid ATTRIBUTE_UNUSED)

return 0; } + +/* + * dnsmasqCapabilities functions - provide useful information about the + * version of dnsmasq on this machine. + * + */ +struct _dnsmasqCaps { + virObject object; + char *binaryPath; + bool noRefresh; + time_t mtime; + virBitmapPtr flags; + unsigned long version; +}; + +static virClassPtr dnsmasqCapsClass; + +static void +dnsmasqCapsDispose(void *obj) +{ + dnsmasqCapsPtr caps = obj; + + virBitmapFree(caps->flags); + VIR_FREE(caps->binaryPath); +} + +static int dnsmasqCapsOnceInit(void) +{ + if (!(dnsmasqCapsClass = virClassNew("dnsmasqCaps", + sizeof(dnsmasqCaps), + dnsmasqCapsDispose))) { + return -1; + } + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(dnsmasqCaps) + +static void +dnsmasqCapsSet(dnsmasqCapsPtr caps, + dnsmasqCapsFlags flag) +{ + ignore_value(virBitmapSetBit(caps->flags, flag)); +} + + +#define SKIP_BLANKS(p) do { while ((*(p) == ' ') || (*(p) == '\t')) (p)++; } while (0) +#define DNSMASQ_VERSION_STR "Dnsmasq version " + +static int +dnsmasqCapsSetFromBuffer(dnsmasqCapsPtr caps, const char *buf) +{ + const char *p; + + caps->noRefresh = true; + + p = buf; + if (!STRPREFIX(p, DNSMASQ_VERSION_STR)) + goto fail; + p += strlen(DNSMASQ_VERSION_STR); + SKIP_BLANKS(p); + if (virParseVersionString(p, &caps->version, true) < 0) + goto fail; + + if (strstr(buf, "--bind-dynamic")) + dnsmasqCapsSet(caps, DNSMASQ_CAPS_BIND_DYNAMIC); + + VIR_INFO("dnsmasq version is %d.%d, --bind-dynamic is %s", + (int)caps->version / 1000000, (int)(caps->version % 1000000) / 1000, + dnsmasqCapsGet(caps, DNSMASQ_CAPS_BIND_DYNAMIC) + ? "present" : "NOT present"); + return 0; + +fail: + p = strchr(buf, '\n'); + if (!p) + p = strchr(buf, '\0'); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse %s version number in '%.*s'"), + caps->binaryPath, (int) (p - buf), buf); + return -1; + +} + +static int +dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path) +{ + int ret = -1; + char *buf = NULL; + + if (virFileReadAll(path, 1024 * 1024, &buf) < 0) + goto cleanup; + + ret = dnsmasqCapsSetFromBuffer(caps, buf); + +cleanup: + VIR_FREE(buf); + return ret; +} + +int +dnsmasqCapsRefresh(dnsmasqCapsPtr caps, bool force) +{ + int ret = -1; + struct stat sb; + virCommandPtr cmd = NULL; + char *help = NULL, *version = NULL, *complete = NULL; + + if (!caps || caps->noRefresh) + return 0; + + if (stat(caps->binaryPath, &sb) < 0) { + virReportSystemError(errno, _("Cannot check dnsmasq binary %s"), + caps->binaryPath); + return -1; + } + if (!force && caps->mtime == sb.st_mtime) { + return 0; + } + caps->mtime = sb.st_mtime; + + /* Make sure the binary we are about to try exec'ing exists. + * Technically we could catch the exec() failure, but that's + * in a sub-process so it's hard to feed back a useful error. + */ + if (!virFileIsExecutable(caps->binaryPath)) { + virReportSystemError(errno, _("dnsmasq binary %s is not executable"), + caps->binaryPath); + goto cleanup; + } + + cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); + virCommandSetOutputBuffer(cmd, &version); + virCommandSetErrorBuffer(cmd, &version); + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(errno, _("failed to run '%s --version': %s"), + caps->binaryPath, version); + goto cleanup; + } + virCommandFree(cmd); + + cmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); + virCommandSetOutputBuffer(cmd, &help); + virCommandSetErrorBuffer(cmd, &help); + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(errno, _("failed to run '%s --help': %s"), + caps->binaryPath, help); + goto cleanup; + } + + if (virAsprintf(&complete, "%s\n%s", version, help) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret = dnsmasqCapsSetFromBuffer(caps, complete); + +cleanup: + virCommandFree(cmd); + VIR_FREE(help); + VIR_FREE(version); + VIR_FREE(complete); + return ret; +} + +static dnsmasqCapsPtr +dnsmasqCapsNewEmpty(const char *binaryPath) +{ + dnsmasqCapsPtr caps; + + if (dnsmasqCapsInitialize() < 0) + return NULL; + if (!(caps = virObjectNew(dnsmasqCapsClass))) + return NULL; + if (!(caps->flags = virBitmapNew(DNSMASQ_CAPS_LAST))) + goto error; + if (!(caps->binaryPath = strdup(binaryPath ? binaryPath : DNSMASQ))) + goto error; + return caps; + +error: + virReportOOMError(); + virObjectUnref(caps); + return NULL; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromBuffer(const char *buf, const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromFile(const char *dataPath, const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsSetFromFile(caps, dataPath) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromBinary(const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsRefresh(caps, true) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +const char * +dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps) +{ + return caps ? caps->binaryPath : DNSMASQ; +} + +unsigned long +dnsmasqCapsGetVersion(dnsmasqCapsPtr caps) +{ + if (caps) + return caps->version; + else + return 0; +} + +bool +dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag) +{ + bool b; + + if (!caps || virBitmapGetBit(caps->flags, flag, &b) < 0) + return false; + else + return b; +} diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h index ad612be..48f0bf9 100644 --- a/src/util/dnsmasq.h +++ b/src/util/dnsmasq.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> * * This library is free software; you can redistribute it and/or @@ -22,6 +22,7 @@ #ifndef __DNSMASQ_H__ # define __DNSMASQ_H__

+# include "virobject.h" # include "virsocketaddr.h"

typedef struct @@ -65,6 +66,16 @@ typedef struct dnsmasqAddnHostsfile *addnhostsfile; } dnsmasqContext;

+typedef enum { + DNSMASQ_CAPS_BIND_DYNAMIC = 0, /* support for --bind-dynamic */ + + DNSMASQ_CAPS_LAST, /* this must always be the last item */ +} dnsmasqCapsFlags; + +typedef struct _dnsmasqCaps dnsmasqCaps; +typedef dnsmasqCaps *dnsmasqCapsPtr; + + dnsmasqContext * dnsmasqContextNew(const char *network_name, const char *config_dir); void dnsmasqContextFree(dnsmasqContext *ctx); @@ -79,4 +90,11 @@ int dnsmasqSave(const dnsmasqContext *ctx); int dnsmasqDelete(const dnsmasqContext *ctx); int dnsmasqReload(pid_t pid);

+dnsmasqCapsPtr dnsmasqCapsNewFromBuffer(const char *buf, const char *binaryPath); +dnsmasqCapsPtr dnsmasqCapsNewFromFile(const char *dataPath, const char *binaryPath); +dnsmasqCapsPtr dnsmasqCapsNewFromBinary(const char *binaryPath); +int dnsmasqCapsRefresh(dnsmasqCapsPtr caps, bool force); +bool dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag); +const char *dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps); +unsigned long dnsmasqCapsGetVersion(dnsmasqCapsPtr caps); #endif /* __DNSMASQ_H__ */ diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 87519e4..69cbd1c 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -46,7 +46,9 @@ static int replaceTokens(char **buf, const char *token, const char *replacement) return 0; }

-static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { +static int +testCompareXMLToArgvFiles(const char *inxml, const char *outargv, dnsmasqCapsPtr caps) +{ char *inXmlData = NULL; char *outArgvData = NULL; char *actual = NULL; @@ -78,7 +80,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (dctx == NULL) goto fail;

- if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx, caps) < 0) goto fail;

if (!(actual = virCommandToString(cmd))) @@ -102,21 +104,27 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { return ret; }

+typedef struct { + const char *name; + dnsmasqCapsPtr caps; +} testInfo; + static int testCompareXMLToArgvHelper(const void *data) { int result = -1; + const testInfo *info = data; char *inxml = NULL; char *outxml = NULL;

if (virAsprintf(&inxml, "%s/networkxml2argvdata/%s.xml", - abs_srcdir, (const char*)data) < 0 || + abs_srcdir, info->name) < 0 || virAsprintf(&outxml, "%s/networkxml2argvdata/%s.argv", - abs_srcdir, (const char*)data) < 0) { + abs_srcdir, info->name) < 0) { goto cleanup; }

- result = testCompareXMLToArgvFiles(inxml, outxml); + result = testCompareXMLToArgvFiles(inxml, outxml, info->caps);

cleanup: VIR_FREE(inxml); @@ -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);

networkDnsmasqLeaseFileName = testDnsmasqLeaseFileName;

-#define DO_TEST(name) \ - if (virtTestRun("Network XML-2-Argv " name, \ - 1, testCompareXMLToArgvHelper, (name)) < 0) \ - ret = -1 - - DO_TEST("isolated-network"); - DO_TEST("routed-network"); - DO_TEST("nat-network"); - DO_TEST("netboot-network"); - DO_TEST("netboot-proxy-network"); - DO_TEST("nat-network-dns-txt-record"); - DO_TEST("nat-network-dns-srv-record"); - DO_TEST("nat-network-dns-srv-record-minimal"); - DO_TEST("nat-network-dns-hosts"); +#define DO_TEST(xname, xcaps) \ + do { \ + static testInfo info; \ + \ + info.name = xname; \ + info.caps = xcaps; \ + if (virtTestRun("Network XML-2-Argv " xname, \ + 1, testCompareXMLToArgvHelper, &info) < 0) { \ + ret = -1; \ + } \ + } while (0) + + DO_TEST("isolated-network", restricted); + DO_TEST("netboot-network", restricted); + DO_TEST("netboot-proxy-network", restricted); + DO_TEST("nat-network-dns-srv-record-minimal", restricted); + DO_TEST("routed-network", full); + DO_TEST("nat-network", full); + DO_TEST("nat-network-dns-txt-record", full); + DO_TEST("nat-network-dns-srv-record", full); + DO_TEST("nat-network-dns-hosts", full);

return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.7.11.7

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 11/22/2012 12:55 AM, Doug Goldstein wrote:

On Nov 21, 2012, at 8:55 PM, Laine Stump <laine@laine.org> 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.

This patch implements a greatly simplified version of the capabilities code we already have for qemu. A dnsmasqCaps device can be created and populated either from running a program on disk, reading a file with the concatenated output of "dnsmasq --version; dnsmasq --help", or examining a buffer in memory that contains the concatenated output of those two commands. Simple functions to retrieve capabilities flags, the version number, and the path of the binary are also included.

bridge_driver.c creates a single dnsmasqCaps object at driver startup, and disposes of it at driver shutdown. Any time it must be used, the dnsmasqCapsRefresh method is called - it checks the mtime of the binary, and re-runs the checks if the binary has changed.

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. --- src/libvirt_private.syms | 8 +- src/network/bridge_driver.c | 59 ++++++---- src/network/bridge_driver.h | 7 +- src/util/dnsmasq.c | 260 ++++++++++++++++++++++++++++++++++++++++++++ src/util/dnsmasq.h | 20 +++- tests/networkxml2argvtest.c | 57 ++++++---- 6 files changed, 366 insertions(+), 45 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a07139..24d2033 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -244,13 +244,19 @@ virDevicePCIAddressParseXML; # dnsmasq.h dnsmasqAddDhcpHost; dnsmasqAddHost; +dnsmasqCapsGet; +dnsmasqCapsGetBinaryPath; +dnsmasqCapsGetVersion; +dnsmasqCapsNewFromBuffer; +dnsmasqCapsNewFromFile; +dnsmasqCapsNewFromBinary; +dnsmasqCapsRefresh; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; dnsmasqReload; dnsmasqSave;

- # domain_audit.h virDomainAuditCgroup; virDomainAuditCgroupMajor; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c153d36..34923ea 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -85,6 +85,7 @@ struct network_driver { char *networkConfigDir; char *networkAutostartDir; char *logDir; + dnsmasqCapsPtr dnsmasqCaps; };

@@ -271,7 +272,8 @@ networkFindActiveConfigs(struct network_driver *driver) { char *radvdpidbase;

ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, obj->def->name, - &obj->dnsmasqPid, DNSMASQ)); + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));

if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) { virReportOOMError(); @@ -389,6 +391,9 @@ networkStartup(int privileged) { goto out_of_memory; }

+ if (!(driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ))) { + /* attempt to continue anyway */ + } Maybe worth a VIR_DEBUG if we are checking for this case.

Well, I could just remove the if() here. I originally *did* have a VIR_WARN in this case, but removed it because dnsmasqCapsNewFromBinary() (actually dnsmasqCapsRefresh) logs either an INFO on success (reporting the version of dnsmasq found and whether or not bind-dynamic is present), or an ERROR on failure - for example if dnsmasq isn't found, it will log this: error : dnsmasqCapsRefresh:703 : Cannot check dnsmasq binary /sbin/dnsmasq: No such file or directory libvirtd still starts up with no problem (as long as it doesn't need to start any networks, in which case it fails those, but still starts). Looking at this showed me that there is a problem with proper version detection if dnsmasq doesn't exist when libvirtd is started, but is installed sometime later, though (it will work, but assumes no capabilities beyond basic existence). I'm going to make a v2 with that fixed.

...

if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, @@ -514,6 +519,8 @@ networkShutdown(void) { if (driverState->iptables) iptablesContextFree(driverState->iptables);

+ virObjectUnref(driverState->dnsmasqCaps); + networkDriverUnlock(driverState); virMutexDestroy(&driverState->lock);

@@ -616,7 +623,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virNetworkIpDefPtr ipdef, const char *pidfile, virCommandPtr cmd, - dnsmasqContext *dctx) + dnsmasqContext *dctx, + dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { int r, ret = -1; int nbleases = 0; @@ -848,7 +856,8 @@ cleanup:

int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, - char *pidfile, dnsmasqContext *dctx) + char *pidfile, dnsmasqContext *dctx, + dnsmasqCapsPtr caps) { virCommandPtr cmd = NULL; int ret = -1, ii; @@ -876,8 +885,8 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;

- cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) { + cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); + if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx, caps) < 0) { goto cleanup; }

@@ -891,7 +900,8 @@ cleanup: }

static int -networkStartDhcpDaemon(virNetworkObjPtr network) +networkStartDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { virCommandPtr cmd = NULL; char *pidfile = NULL; @@ -935,7 +945,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); if (ret < 0) goto cleanup;

@@ -988,7 +1000,8 @@ cleanup: * Returns 0 on success, -1 on failure. */ static int -networkRefreshDhcpDaemon(virNetworkObjPtr network) +networkRefreshDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { int ret = -1, ii; virNetworkIpDefPtr ipdef; @@ -996,7 +1009,7 @@ networkRefreshDhcpDaemon(virNetworkObjPtr network)

/* if there's no running dnsmasq, just start it */ if (network->dnsmasqPid <= 0 || (kill(network->dnsmasqPid, 0) < 0)) - return networkStartDhcpDaemon(network); + return networkStartDhcpDaemon(driver, network);

/* Look for first IPv4 address that has dhcp defined. */ /* We support dhcp config on 1 IPv4 interface only. */ @@ -1038,7 +1051,8 @@ cleanup: * Returns 0 on success, -1 on failure. */ static int -networkRestartDhcpDaemon(virNetworkObjPtr network) +networkRestartDhcpDaemon(struct network_driver *driver, + virNetworkObjPtr network) { /* if there is a running dnsmasq, kill it */ if (network->dnsmasqPid > 0) { @@ -1047,7 +1061,7 @@ networkRestartDhcpDaemon(virNetworkObjPtr network) network->dnsmasqPid = -1; } /* now start dnsmasq if it should be started */ - return networkStartDhcpDaemon(network); + return networkStartDhcpDaemon(driver, network); }

static int @@ -1245,7 +1259,8 @@ cleanup: }

static int -networkRefreshRadvd(virNetworkObjPtr network) +networkRefreshRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network) { /* if there's no running radvd, just start it */ if (network->radvdPid <= 0 || (kill(network->radvdPid, 0) < 0)) @@ -1265,7 +1280,8 @@ networkRefreshRadvd(virNetworkObjPtr network) #if 0 /* currently unused, so it causes a build error unless we #if it out */ static int -networkRestartRadvd(virNetworkObjPtr network) +networkRestartRadvd(struct network_driver *driver, + virNetworkObjPtr network) { char *radvdpidbase;

@@ -1313,8 +1329,8 @@ networkRefreshDaemons(struct network_driver *driver) * dnsmasq and/or radvd, or restart them if they've * disappeared. */ - networkRefreshDhcpDaemon(network); - networkRefreshRadvd(network); + networkRefreshDhcpDaemon(driver, network); + networkRefreshRadvd(driver, network); } virNetworkObjUnlock(network); } @@ -2224,7 +2240,8 @@ networkStartNetworkVirtual(struct network_driver *driver,

/* start dnsmasq if there are any IP addresses (v4 or v6) */ - if ((v4present || v6present) && networkStartDhcpDaemon(network) < 0) + if ((v4present || v6present) && + networkStartDhcpDaemon(driver, network) < 0) goto err3;

/* start radvd if there are any ipv6 addresses */ @@ -2988,7 +3005,7 @@ networkUpdate(virNetworkPtr net, /* these sections all change things on the dnsmasq commandline, * so we need to kill and restart dnsmasq. */ - if (networkRestartDhcpDaemon(network) < 0) + if (networkRestartDhcpDaemon(driver,network) < 0)

Small nit. Missing a space after the ,

...

goto cleanup;

} else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST) { @@ -3009,8 +3026,8 @@ networkUpdate(virNetworkPtr net, }

if ((newDhcpActive != oldDhcpActive && - networkRestartDhcpDaemon(network) < 0) || - networkRefreshDhcpDaemon(network) < 0) { + networkRestartDhcpDaemon(driver, network) < 0) || + networkRefreshDhcpDaemon(driver, network) < 0) { goto cleanup; }

@@ -3021,7 +3038,7 @@ networkUpdate(virNetworkPtr net, * can just update the config files and send SIGHUP to * dnsmasq. */ - if (networkRefreshDhcpDaemon(network) < 0) + if (networkRefreshDhcpDaemon(driver, network) < 0) goto cleanup;

} @@ -3030,7 +3047,7 @@ networkUpdate(virNetworkPtr net, /* only a change in IP addresses will affect radvd, and all of radvd's * config is stored in the conf file which will be re-read with a SIGHUP. */ - if (networkRefreshRadvd(network) < 0) + if (networkRefreshRadvd(driver, network) < 0) goto cleanup; }

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 0fae275..3af74a1 100644 --- a/src/network/bridge_driver.h +++ 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.

Liberal with the dates. Did Red Hat contribute in 2008 to 2010 to this file?

Well, I was following advice that I thought I remembered that "it's okay to squash a disjoint set of years in a Copyright to a single range". After asking around, I'm now thinking that I may have mis-remembered what I was told, but as it turns out this file has been modified by someone from Red Hat at least a couple of times during every year back to 2009, when it was moved from src/network_driver.h, and modified in that location in 2008 and 2007, when it was split off from src/qemu_conf.h. So I squashed the dates based on suspect reasoning, but the result actually is correct :-)

...

* Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -48,7 +48,8 @@ int networkGetNetworkAddress(const char *netname, char **netaddr)

int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, - dnsmasqContext *dctx) + dnsmasqContext *dctx, + dnsmasqCapsPtr caps) ; # else /* Define no-op replacements that don't drag in any link dependencies. */ @@ -56,7 +57,7 @@ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx, caps) 0 # endif

typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index 9d1c07b..03b3792 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -39,8 +39,10 @@

#include "internal.h" #include "datatypes.h" +#include "bitmap.h" #include "dnsmasq.h" #include "util.h" +#include "command.h" #include "memory.h" #include "virterror_internal.h" #include "logging.h" @@ -583,3 +585,261 @@ dnsmasqReload(pid_t pid ATTRIBUTE_UNUSED)

return 0; } + +/* + * dnsmasqCapabilities functions - provide useful information about the + * version of dnsmasq on this machine. + * + */ +struct _dnsmasqCaps { + virObject object; + char *binaryPath; + bool noRefresh; + time_t mtime; + virBitmapPtr flags; + unsigned long version; +}; + +static virClassPtr dnsmasqCapsClass; + +static void +dnsmasqCapsDispose(void *obj) +{ + dnsmasqCapsPtr caps = obj; + + virBitmapFree(caps->flags); + VIR_FREE(caps->binaryPath); +} + +static int dnsmasqCapsOnceInit(void) +{ + if (!(dnsmasqCapsClass = virClassNew("dnsmasqCaps", + sizeof(dnsmasqCaps), + dnsmasqCapsDispose))) { + return -1; + } + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(dnsmasqCaps) + +static void +dnsmasqCapsSet(dnsmasqCapsPtr caps, + dnsmasqCapsFlags flag) +{ + ignore_value(virBitmapSetBit(caps->flags, flag)); +} + + +#define SKIP_BLANKS(p) do { while ((*(p) == ' ') || (*(p) == '\t')) (p)++; } while (0) Do we need to make sure the command below is executed with LC_ALL=C or call virCommandAddEnvPassCommon()?

Ah yes, good catch! I based this code on the qemu capabilities functions, but hadn't noticed that they were calling a separate function to create the virCommand, and that function does a couple of useful things: virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); I'll do both of those in the next revision.

...

+#define DNSMASQ_VERSION_STR "Dnsmasq version " + +static int +dnsmasqCapsSetFromBuffer(dnsmasqCapsPtr caps, const char *buf) +{ + const char *p; + + caps->noRefresh = true; + + p = buf; + if (!STRPREFIX(p, DNSMASQ_VERSION_STR)) + goto fail; + p += strlen(DNSMASQ_VERSION_STR); + SKIP_BLANKS(p); + if (virParseVersionString(p, &caps->version, true) < 0) + goto fail; + + if (strstr(buf, "--bind-dynamic")) + dnsmasqCapsSet(caps, DNSMASQ_CAPS_BIND_DYNAMIC); + + VIR_INFO("dnsmasq version is %d.%d, --bind-dynamic is %s", + (int)caps->version / 1000000, (int)(caps->version % 1000000) / 1000, + dnsmasqCapsGet(caps, DNSMASQ_CAPS_BIND_DYNAMIC) + ? "present" : "NOT present"); + return 0; + +fail: + p = strchr(buf, '\n'); + if (!p) + p = strchr(buf, '\0'); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse %s version number in '%.*s'"), + caps->binaryPath, (int) (p - buf), buf); + return -1; + +} + +static int +dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path) +{ + int ret = -1; + char *buf = NULL; + + if (virFileReadAll(path, 1024 * 1024, &buf) < 0) + goto cleanup; + + ret = dnsmasqCapsSetFromBuffer(caps, buf); + +cleanup: + VIR_FREE(buf); + return ret; +} + +int +dnsmasqCapsRefresh(dnsmasqCapsPtr caps, bool force) +{ + int ret = -1; + struct stat sb; + virCommandPtr cmd = NULL; + char *help = NULL, *version = NULL, *complete = NULL; + + if (!caps || caps->noRefresh) + return 0; + + if (stat(caps->binaryPath, &sb) < 0) { + virReportSystemError(errno, _("Cannot check dnsmasq binary %s"), + caps->binaryPath); + return -1; + } + if (!force && caps->mtime == sb.st_mtime) { + return 0; + } + caps->mtime = sb.st_mtime; + + /* Make sure the binary we are about to try exec'ing exists. + * Technically we could catch the exec() failure, but that's + * in a sub-process so it's hard to feed back a useful error. + */ + if (!virFileIsExecutable(caps->binaryPath)) { Isn't this another stat()? We could just use the results of the previous stat().

This bit of code was copied directly from qemu_capabilities.c. As infrequently as it's executed (normally only once each time libvirtd is started), I'd rather leave it this was as it does a better job of self-documenting than the alternative.

...

+ virReportSystemError(errno, _("dnsmasq binary %s is not executable"), + caps->binaryPath); + goto cleanup; + } + + cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); + virCommandSetOutputBuffer(cmd, &version); + virCommandSetErrorBuffer(cmd, &version); + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(errno, _("failed to run '%s --version': %s"), + caps->binaryPath, version); + goto cleanup; + } + virCommandFree(cmd); + + cmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); + virCommandSetOutputBuffer(cmd, &help); + virCommandSetErrorBuffer(cmd, &help); + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(errno, _("failed to run '%s --help': %s"), + caps->binaryPath, help); + goto cleanup; + } + + if (virAsprintf(&complete, "%s\n%s", version, help) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret = dnsmasqCapsSetFromBuffer(caps, complete); + +cleanup: + virCommandFree(cmd); + VIR_FREE(help); + VIR_FREE(version); + VIR_FREE(complete); + return ret; +} + +static dnsmasqCapsPtr +dnsmasqCapsNewEmpty(const char *binaryPath) +{ + dnsmasqCapsPtr caps; + + if (dnsmasqCapsInitialize() < 0) + return NULL; + if (!(caps = virObjectNew(dnsmasqCapsClass))) + return NULL; + if (!(caps->flags = virBitmapNew(DNSMASQ_CAPS_LAST))) + goto error; + if (!(caps->binaryPath = strdup(binaryPath ? binaryPath : DNSMASQ))) + goto error; + return caps; + +error: + virReportOOMError(); + virObjectUnref(caps); + return NULL; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromBuffer(const char *buf, const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromFile(const char *dataPath, const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsSetFromFile(caps, dataPath) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +dnsmasqCapsPtr +dnsmasqCapsNewFromBinary(const char *binaryPath) +{ + dnsmasqCapsPtr caps = dnsmasqCapsNewEmpty(binaryPath); + + if (!caps) + return NULL; + + if (dnsmasqCapsRefresh(caps, true) < 0) { + virObjectUnref(caps); + return NULL; + } + return caps; +} + +const char * +dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps) +{ + return caps ? caps->binaryPath : DNSMASQ; +} + +unsigned long +dnsmasqCapsGetVersion(dnsmasqCapsPtr caps) +{ + if (caps) + return caps->version; + else + return 0; +} + +bool +dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag) +{ + bool b; + + if (!caps || virBitmapGetBit(caps->flags, flag, &b) < 0) + return false; + else + return b; +} diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h index ad612be..48f0bf9 100644 --- a/src/util/dnsmasq.h +++ b/src/util/dnsmasq.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. Bit liberal on the date change. I assume Red Hat contributed in 2011 but no one updated it.

Yes. Again I made the change based on incorrect knowledge, but I checked just now and found that it was changed multiple times during 2011 by a Red Hat employee.

...

* Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> * * This library is free software; you can redistribute it and/or @@ -22,6 +22,7 @@ #ifndef __DNSMASQ_H__ # define __DNSMASQ_H__

+# include "virobject.h" # include "virsocketaddr.h"

typedef struct @@ -65,6 +66,16 @@ typedef struct dnsmasqAddnHostsfile *addnhostsfile; } dnsmasqContext;

+typedef enum { + DNSMASQ_CAPS_BIND_DYNAMIC = 0, /* support for --bind-dynamic */ + + DNSMASQ_CAPS_LAST, /* this must always be the last item */ +} dnsmasqCapsFlags; + +typedef struct _dnsmasqCaps dnsmasqCaps; +typedef dnsmasqCaps *dnsmasqCapsPtr; + + dnsmasqContext * dnsmasqContextNew(const char *network_name, const char *config_dir); void dnsmasqContextFree(dnsmasqContext *ctx); @@ -79,4 +90,11 @@ int dnsmasqSave(const dnsmasqContext *ctx); int dnsmasqDelete(const dnsmasqContext *ctx); int dnsmasqReload(pid_t pid);

+dnsmasqCapsPtr dnsmasqCapsNewFromBuffer(const char *buf, const char *binaryPath); +dnsmasqCapsPtr dnsmasqCapsNewFromFile(const char *dataPath, const char *binaryPath); +dnsmasqCapsPtr dnsmasqCapsNewFromBinary(const char *binaryPath); +int dnsmasqCapsRefresh(dnsmasqCapsPtr caps, bool force); +bool dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag); +const char *dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps); +unsigned long dnsmasqCapsGetVersion(dnsmasqCapsPtr caps); #endif /* __DNSMASQ_H__ */ diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 87519e4..69cbd1c 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -46,7 +46,9 @@ static int replaceTokens(char **buf, const char *token, const char *replacement) return 0; }

-static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { +static int +testCompareXMLToArgvFiles(const char *inxml, const char *outargv, dnsmasqCapsPtr caps) +{ char *inXmlData = NULL; char *outArgvData = NULL; char *actual = NULL; @@ -78,7 +80,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (dctx == NULL) goto fail;

- if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx, caps) < 0) goto fail;

if (!(actual = virCommandToString(cmd))) @@ -102,21 +104,27 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { return ret; }

+typedef struct { + const char *name; + dnsmasqCapsPtr caps; +} testInfo; + static int testCompareXMLToArgvHelper(const void *data) { int result = -1; + const testInfo *info = data; char *inxml = NULL; char *outxml = NULL;

if (virAsprintf(&inxml, "%s/networkxml2argvdata/%s.xml", - abs_srcdir, (const char*)data) < 0 || + abs_srcdir, info->name) < 0 || virAsprintf(&outxml, "%s/networkxml2argvdata/%s.argv", - abs_srcdir, (const char*)data) < 0) { + abs_srcdir, info->name) < 0) { goto cleanup; }

- result = testCompareXMLToArgvFiles(inxml, outxml); + result = testCompareXMLToArgvFiles(inxml, outxml, info->caps);

cleanup: VIR_FREE(inxml); @@ -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);

networkDnsmasqLeaseFileName = testDnsmasqLeaseFileName;

-#define DO_TEST(name) \ - if (virtTestRun("Network XML-2-Argv " name, \ - 1, testCompareXMLToArgvHelper, (name)) < 0) \ - ret = -1 - - DO_TEST("isolated-network"); - DO_TEST("routed-network"); - DO_TEST("nat-network"); - DO_TEST("netboot-network"); - DO_TEST("netboot-proxy-network"); - DO_TEST("nat-network-dns-txt-record"); - DO_TEST("nat-network-dns-srv-record"); - DO_TEST("nat-network-dns-srv-record-minimal"); - DO_TEST("nat-network-dns-hosts"); +#define DO_TEST(xname, xcaps) \ + do { \ + static testInfo info; \ + \ + info.name = xname; \ + info.caps = xcaps; \ + if (virtTestRun("Network XML-2-Argv " xname, \ + 1, testCompareXMLToArgvHelper, &info) < 0) { \ + ret = -1; \ + } \ + } while (0) + + DO_TEST("isolated-network", restricted); + DO_TEST("netboot-network", restricted); + DO_TEST("netboot-proxy-network", restricted); + DO_TEST("nat-network-dns-srv-record-minimal", restricted); + DO_TEST("routed-network", full); + DO_TEST("nat-network", full); + DO_TEST("nat-network-dns-txt-record", full); + DO_TEST("nat-network-dns-srv-record", full); + DO_TEST("nat-network-dns-hosts", full);

return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.7.11.7 Overall looks good. Just had a few questions. Unfortunately I'm on an iPad with no source around so apologies if they are dumb.

No, you made a couple of very useful points :-) Thanks!

On Nov 21, 2012, at 8:55 PM, Laine Stump <laine@laine.org> wrote:

This new function returns true if the given address is in the range of any "private" or "local" networks as defined in RFC1918 (IPv4) or RFC4193 (IPv6), otherwise they return false.

These ranges are:

192.168.0.0/16 172.16.0.0/16 10.0.0.0/24 FC00::/7 --- src/libvirt_private.syms | 1 + src/util/virsocketaddr.c | 32 +++++++++++++++++++++++++++++++- src/util/virsocketaddr.h | 3 ++- 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 24d2033..85bff2c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1776,6 +1776,7 @@ virSocketAddrFormatFull; virSocketAddrGetPort; virSocketAddrGetRange; virSocketAddrIsNetmask; +virSocketAddrIsPrivate; virSocketAddrMask; virSocketAddrMaskByPrefix; virSocketAddrParse; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index b6cc16f..2d39458 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -194,6 +194,36 @@ virSocketAddrEqual(const virSocketAddrPtr s1, const virSocketAddrPtr s2) }

/* + * virSocketAddrIsPrivate: + * @s: the location of the IP address + * + * Return true if this address is in its family's defined + * "private/local" address space. For IPv4, private addresses are in + * the range of 192.168.0.0/16, 172.16.0.0/16, or 10.0.0.0/8. For + * IPv6, local addresses are in the range of FC00::/7. + * + * See RFC1918 and RFC4193 for details. + */ +bool +virSocketAddrIsPrivate(const virSocketAddrPtr addr) +{ + unsigned long val; + + switch (addr->data.stor.ss_family) { + case AF_INET: + val = ntohl(addr->data.inet4.sin_addr.s_addr); + + return ((val & 0xFFFF0000) == ((192L << 24) + (168 << 16)) || + (val & 0xFFFF0000) == ((172L << 24) + (16 << 16)) || + (val & 0xFF000000) == ((10L << 24))); + + case AF_INET6: + return (addr->data.inet6.sin6_addr.s6_addr[0] & 0xFC) == 0xFC; + } + return false; +} + +/* * virSocketAddrFormat: * @addr: an initialized virSocketAddrPtr * diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 51ddd2d..66d4265 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -104,5 +104,6 @@ int virSocketAddrPrefixToNetmask(unsigned int prefix, int family); bool virSocketAddrEqual(const virSocketAddrPtr s1, const virSocketAddrPtr s2); +bool virSocketAddrIsPrivate(const virSocketAddrPtr addr);

#endif /* __VIR_SOCKETADDR_H__ */ -- 1.7.11.7

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

ACK. Looks good. -- Doug

On Nov 21, 2012, at 8:55 PM, Laine Stump <laine@laine.org> wrote:

This bug resolves CVE-2012-3411, which is described in the following bugzilla report:

https://bugzilla.redhat.com/show_bug.cgi?id=833033

The following report is specifically for libvirt on Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=874702

In short, a dnsmasq instance run with the intention of listening for DHCP/DNS requests only on a libvirt virtual network (which is constructed using a Linux host bridge) would also answer queries sent from outside the virtualization host.

This patch takes advantage of a new dnsmasq option "--bind-dynamic", which will cause the listening socket to be setup such that it will only receive those requests that actually come in via the bridge interface. In order for this behavior to actually occur, not only must "--bind-interfaces" be replaced with "--bind-dynamic", but also all "--listen-address" options must be replaced with a single "--interface" option. Fully:

--bind-interfaces --except-interface lo --listen-address x.x.x.x ...

(with --listen-address possibly repeated) is replaced with:

--bind-dynamic --interface virbrX

Of course libvirt can't use this new option if the host's dnsmasq doesn't have it, but we still want libvirt to function (because the great majority of libvirt installations, which only have mode='nat' networks using RFC1918 private address ranges (e.g. 192.168.122.0/24), are immune to this vulnerability from anywhere beyond the local subnet of the host), so we use the new dnsmasqCaps API to check if dnsmasq supports the new option and, if not, we use the "old" option style instead. In order to assure that this permissiveness doesn't lead to a vulnerable system, we do check for non-private addresses in this case, and refuse to start the network if both a) we are using the old-style options, and b) the network has a publicly routable IP address. Hopefully this will provide the proper balance of not being disruptive to those not practically affected, and making sure that those who *are* affected get their dnsmasq upgraded.

(--bind-dynamic was added to dnsmasq in upstream commit 54dd393f3938fc0c19088fbd319b95e37d81a2b0, which was included in dnsmasq-2.63) --- src/network/bridge_driver.c | 77 +++++++++++++++------- tests/networkxml2argvdata/isolated-network.argv | 5 +- .../networkxml2argvdata/nat-network-dns-hosts.argv | 5 +- .../nat-network-dns-srv-record-minimal.argv | 9 ++- .../nat-network-dns-srv-record-minimal.xml | 4 +- .../nat-network-dns-srv-record.argv | 8 +-- .../nat-network-dns-txt-record.argv | 8 +-- tests/networkxml2argvdata/nat-network.argv | 6 +- tests/networkxml2argvdata/netboot-network.argv | 4 +- .../networkxml2argvdata/netboot-proxy-network.argv | 5 +- tests/networkxml2argvdata/routed-network.argv | 4 +- 11 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 34923ea..b3eb11c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -658,7 +658,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * Needed to ensure dnsmasq uses same algorithm for processing * multiple namedriver entries in /etc/resolv.conf as GLibC. */ - virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL); + virCommandAddArg(cmd, "--strict-order");

if (network->def->domain) virCommandAddArgPair(cmd, "--domain", network->def->domain); @@ -673,9 +673,60 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, /* *no* conf file */ virCommandAddArg(cmd, "--conf-file=");

- virCommandAddArgList(cmd, - "--except-interface", "lo", - NULL); + if (dnsmasqCapsGet(caps, DNSMASQ_CAPS_BIND_DYNAMIC)) { + /* using --bind-dynamic with only --interface (no + * --listen-address) prevents dnsmasq from responding to dns + * queries that arrive on some interface other than our bridge + * interface (in other words, requests originating somewhere + * other than one of the virtual guests connected directly to + * this network). This was added in response to CVE 2012-3411. + */ + virCommandAddArgList(cmd, + "--bind-dynamic", + "--interface", network->def->bridge, + NULL); + } else { + virCommandAddArgList(cmd, + "--bind-interfaces", + "--except-interface", "lo", + NULL); + /* + * --interface does not actually work with dnsmasq < 2.47, + * due to DAD for ipv6 addresses on the interface. + * + * virCommandAddArgList(cmd, "--interface", network->def->bridge, NULL); + * + * So listen on all defined IPv[46] addresses + */ + for (ii = 0; + (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); + ii++) { + char *ipaddr = virSocketAddrFormat(&tmpipdef->address); + + if (!ipaddr) + goto cleanup; + /* also part of CVE 2012-3411 - if the host's version of + * dnsmasq doesn't have --bind-dynamic, only allow listening on + * private/local IP addresses (see RFC1918/RFC4193) + */ + if (!virSocketAddrIsPrivate(&tmpipdef->address)) { + unsigned long version = dnsmasqCapsGetVersion(caps); + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Publicly routable address %s is prohibited. " + "The version of dnsmasq on this host (%d.%d) doesn't " + "support the --bind-dynamic option, which is required " + "for safe operation on a publicly routable subnet " + "(see CVE-2012-3411). You must either upgrade dnsmasq, " + "or use a private/local subnet range for this network " + "(as described in RFC1918/RFC4193)."), ipaddr, + (int)version / 1000000, (int)(version % 1000000) / 1000); + goto cleanup; + } + virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); + VIR_FREE(ipaddr); + } + }

/* If this is an isolated network, set the default route option * (3) to be empty to avoid setting a default route that's @@ -741,24 +792,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, } }

- /* - * --interface does not actually work with dnsmasq < 2.47, - * due to DAD for ipv6 addresses on the interface. - * - * virCommandAddArgList(cmd, "--interface", ipdef->bridge, NULL); - * - * So listen on all defined IPv[46] addresses - */ - for (ii = 0; - (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { - char *ipaddr = virSocketAddrFormat(&tmpipdef->address); - if (!ipaddr) - goto cleanup; - virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); - VIR_FREE(ipaddr); - } - if (ipdef) { for (r = 0 ; r < ipdef->nranges ; r++) { char *saddr = virSocketAddrFormat(&ipdef->ranges[r].start); diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 13e77b2..3d8601e 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,7 +1,8 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ +@DNSMASQ@ --strict-order \ --local=// --domain-needed --conf-file= \ ---except-interface lo --dhcp-option=3 --no-resolv \ +--bind-interfaces --except-interface lo \ --listen-address 192.168.152.1 \ +--dhcp-option=3 --no-resolv \ --dhcp-range 192.168.152.2,192.168.152.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 \ --dhcp-no-override \ diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 03a0676..e5143ac 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,4 +1,5 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ +@DNSMASQ@ --strict-order --domain=example.com \ --local=/example.com/ --domain-needed \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ +--conf-file= \ +--bind-dynamic --interface virbr0 \ --expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 210a60c..031da3f 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,14 +1,13 @@ @DNSMASQ@ \ --strict-order \ ---bind-interfaces \ --local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.,,,, \ +--bind-interfaces --except-interface lo \ --listen-address 192.168.122.1 \ --listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---listen-address 2001:db8:ac10:fd01::1 \ +--listen-address fc00:db8:ac10:fe01::1 \ +--listen-address fc00:db8:ac10:fd01::1 \ --listen-address 10.24.10.1 \ +--srv-host=name.tcp.,,,, \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ --dhcp-lease-max=253 \ diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.xml b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.xml index e9b7680..f6f24e1 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.xml +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.xml @@ -17,9 +17,9 @@ </ip> <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> </ip> - <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> + <ip family='ipv6' address='fc00:db8:ac10:fe01::1' prefix='64'> </ip> - <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <ip family='ipv6' address='fc00:db8:ac10:fd01::1' prefix='64'> </ip> <ip family='ipv4' address='10.24.10.1'> </ip> diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index 833d3cd..beff591 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,14 +1,8 @@ @DNSMASQ@ \ --strict-order \ ---bind-interfaces \ --local=// --domain-needed --conf-file= \ ---except-interface lo \ +--bind-dynamic --interface virbr0 \ --srv-host=name.tcp.test-domain-name,.,1024,10,10 \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---listen-address 2001:db8:ac10:fd01::1 \ ---listen-address 10.24.10.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ --dhcp-lease-max=253 \ diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 3481507..fc164f6 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,9 +1,7 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ +@DNSMASQ@ --strict-order \ --local=// --domain-needed --conf-file= \ ---except-interface lo '--txt-record=example,example value' \ ---listen-address 192.168.122.1 --listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \ +--bind-dynamic --interface virbr0 \ +'--txt-record=example,example value' \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ --dhcp-lease-max=253 --dhcp-no-override \ diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index 37fd2fc..6303f76 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,8 +1,6 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ +@DNSMASQ@ --strict-order \ --local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ ---listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \ +--bind-dynamic --interface virbr0 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ --dhcp-lease-max=253 --dhcp-no-override \ diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 5408eb7..9699e5c 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,6 +1,6 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ +@DNSMASQ@ --strict-order --domain=example.com \ --local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ +--bind-interfaces --except-interface lo --listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ --dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 21e01e3..9ac3018 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,6 +1,7 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ +@DNSMASQ@ --strict-order --domain=example.com \ --local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ +--bind-interfaces --except-interface lo \ +--listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ --dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index 9fedb2b..700c904 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,4 +1,4 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ +@DNSMASQ@ --strict-order \ --local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ +--bind-dynamic --interface virbr1 \ --addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts\ -- 1.7.11.7

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

ACK. This all looks good. -- Doug