[libvirt] [PATCH] Convert dhcpStartDhcpDaemon from virRun to virCommand
This is pretty straightforward - even though dnsmasq gets daemonized and uses a pid file, those things are both handled by the dnsmasq binary itself. And libvirt doesn't need any of the output of the dnsmasq command either, so we just setup the args and call virRun(). Mainly it was just a (mostly) mechanical job of replacing the APPEND_ARG() macro (and some other *printfs()) with virCommandAddArg*(). --- src/network/bridge_driver.c | 238 +++++++++++++++---------------------------- 1 files changed, 80 insertions(+), 158 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 62639e4..9802222 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -51,6 +51,7 @@ #include "event.h" #include "buf.h" #include "util.h" +#include "command.h" #include "memory.h" #include "uuid.h" #include "iptables.h" @@ -390,25 +391,15 @@ networkSaveDnsmasqHostsfile(virNetworkObjPtr network, static int networkBuildDnsmasqArgv(virNetworkObjPtr network, const char *pidfile, - const char ***argv) { - int i, len, r; + virCommandPtr cmd) { + int r, ret = -1; int nbleases = 0; - char *pidfileArg; - char buf[1024]; - unsigned int ranges; - char *ipAddr; - - /* - * For static-only DHCP, i.e. with no range but at least one host element, - * we have to add a special --dhcp-range option to enable the service in - * dnsmasq. - */ - ranges = network->def->nranges; - if (!ranges && network->def->nhosts) - ranges = 1; + char *bridgeaddr; + if (!(bridgeaddr = virSocketFormatAddr(&network->def->ipAddress))) + goto cleanup; /* - * NB, be careful about syntax for dnsmasq options in long format + * NB, be careful about syntax for dnsmasq options in long format. * * If the flag has a mandatory argument, it can be given using * either syntax: @@ -422,66 +413,27 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * --foo=bar * * It is hard to determine whether a flag is optional or not, - * without reading the dnsmasq source :-( The manpages is not - * very explicit on this + * without reading the dnsmasq source :-( The manpage is not + * very explicit on this. */ - len = - 1 + /* dnsmasq */ - 1 + /* --strict-order */ - 1 + /* --bind-interfaces */ - (network->def->domain?2:0) + /* --domain name */ - 2 + /* --pid-file /var/run/libvirt/network/$NAME.pid */ - 2 + /* --conf-file "" */ - /*2 + *//* --interface virbr0 */ - 2 + /* --except-interface lo */ - 2 + /* --listen-address 10.0.0.1 */ - (2 * ranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */ - /* --dhcp-lease-max=xxx if needed */ - (network->def->nranges ? 1 : 0) + - /* --dhcp-no-override if needed */ - (ranges ? 1 : 0) + - /* --dhcp-hostsfile=/var/lib/dnsmasq/$NAME.hostsfile */ - (network->def->nhosts > 0 ? 1 : 0) + - /* --enable-tftp --tftp-root /srv/tftp */ - (network->def->tftproot ? 3 : 0) + - /* --dhcp-boot pxeboot.img[,,12.34.56.78] */ - (network->def->bootfile ? 2 : 0) + - 1; /* NULL */ - - if (VIR_ALLOC_N(*argv, len) < 0) - goto no_memory; - -#define APPEND_ARG(v, n, s) do { \ - if (!((v)[(n)] = strdup(s))) \ - goto no_memory; \ - } while (0) - -#define APPEND_ARG_LIT(v, n, s) \ - (v)[(n)] = s - - i = 0; - - APPEND_ARG(*argv, i++, DNSMASQ); - /* * Needed to ensure dnsmasq uses same algorithm for processing * multiple namedriver entries in /etc/resolv.conf as GLibC. */ - APPEND_ARG(*argv, i++, "--strict-order"); - APPEND_ARG(*argv, i++, "--bind-interfaces"); + virCommandAddArg(cmd, "--strict-order"); + virCommandAddArg(cmd, "--bind-interfaces"); if (network->def->domain) { - APPEND_ARG(*argv, i++, "--domain"); - APPEND_ARG(*argv, i++, network->def->domain); + virCommandAddArg(cmd, "--domain"); + virCommandAddArg(cmd, network->def->domain); } - if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile) < 0) - goto no_memory; - APPEND_ARG_LIT(*argv, i++, pidfileArg); + virCommandAddArgPair(cmd, "--pid-file", pidfile); - APPEND_ARG(*argv, i++, "--conf-file="); - APPEND_ARG(*argv, i++, ""); + /* *no* conf file */ + virCommandAddArg(cmd, "--conf-file="); + virCommandAddArg(cmd, ""); /* * XXX does not actually work, due to some kind of @@ -489,166 +441,140 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * interface. A sleep(10) makes it work, but that's * clearly not practical * - * APPEND_ARG(*argv, i++, "--interface"); - * APPEND_ARG(*argv, i++, network->def->bridge); + * virCommandAddArg(cmd, "--interface"); + * virCommandAddArg(cmd, network->def->bridge); */ - APPEND_ARG(*argv, i++, "--listen-address"); - if (!(ipAddr = virSocketFormatAddr(&network->def->ipAddress))) - goto error; - APPEND_ARG_LIT(*argv, i++, ipAddr); + virCommandAddArg(cmd, "--listen-address"); + virCommandAddArg(cmd, bridgeaddr); - APPEND_ARG(*argv, i++, "--except-interface"); - APPEND_ARG(*argv, i++, "lo"); + virCommandAddArg(cmd, "--except-interface"); + virCommandAddArg(cmd, "lo"); for (r = 0 ; r < network->def->nranges ; r++) { char *saddr = virSocketFormatAddr(&network->def->ranges[r].start); if (!saddr) - goto error; + goto cleanup; char *eaddr = virSocketFormatAddr(&network->def->ranges[r].end); if (!eaddr) { VIR_FREE(saddr); - goto error; + goto cleanup; } - char *range; - int rc = virAsprintf(&range, "%s,%s", saddr, eaddr); + virCommandAddArg(cmd, "--dhcp-range"); + virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); - if (rc < 0) - goto no_memory; - APPEND_ARG(*argv, i++, "--dhcp-range"); - APPEND_ARG_LIT(*argv, i++, range); nbleases += virSocketGetRange(&network->def->ranges[r].start, &network->def->ranges[r].end); } + /* + * For static-only DHCP, i.e. with no range but at least one host element, + * we have to add a special --dhcp-range option to enable the service in + * dnsmasq. + */ if (!network->def->nranges && network->def->nhosts) { - char *ipaddr = virSocketFormatAddr(&network->def->ipAddress); - if (!ipaddr) - goto error; - char *range; - int rc = virAsprintf(&range, "%s,static", ipaddr); - VIR_FREE(ipaddr); - if (rc < 0) - goto no_memory; - - APPEND_ARG(*argv, i++, "--dhcp-range"); - APPEND_ARG_LIT(*argv, i++, range); + virCommandAddArg(cmd, "--dhcp-range"); + virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); } if (network->def->nranges > 0) { - snprintf(buf, sizeof(buf), "--dhcp-lease-max=%d", nbleases); - APPEND_ARG(*argv, i++, buf); + virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); } - if (ranges) - APPEND_ARG(*argv, i++, "--dhcp-no-override"); + if (network->def->nranges || network->def->nhosts) + virCommandAddArg(cmd, "--dhcp-no-override"); if (network->def->nhosts > 0) { - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); - char *hostsfileArg; - - if (dctx == NULL) - goto no_memory; + dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, + DNSMASQ_STATE_DIR); + if (dctx == NULL) { + virReportOOMError(); + goto cleanup; + } if (networkSaveDnsmasqHostsfile(network, dctx, false)) { - if (virAsprintf(&hostsfileArg, "--dhcp-hostsfile=%s", dctx->hostsfile->path) < 0) { - dnsmasqContextFree(dctx); - goto no_memory; - } - APPEND_ARG_LIT(*argv, i++, hostsfileArg); + virCommandAddArgPair(cmd, "--dhcp-hostsfile", + dctx->hostsfile->path); } - dnsmasqContextFree(dctx); } if (network->def->tftproot) { - APPEND_ARG(*argv, i++, "--enable-tftp"); - APPEND_ARG(*argv, i++, "--tftp-root"); - APPEND_ARG(*argv, i++, network->def->tftproot); + virCommandAddArg(cmd, "--enable-tftp"); + virCommandAddArg(cmd, "--tftp-root"); + virCommandAddArg(cmd, network->def->tftproot); } if (network->def->bootfile) { - char *ipaddr = NULL; + + virCommandAddArg(cmd, "--dhcp-boot"); if (VIR_SOCKET_HAS_ADDR(&network->def->bootserver)) { - if (!(ipaddr = virSocketFormatAddr(&network->def->bootserver))) - goto error; - } - char *boot; - int rc = virAsprintf(&boot, "%s%s%s", - network->def->bootfile, - ipaddr ? ",," : "", - ipaddr ? ipaddr : ""); - VIR_FREE(ipaddr); - if (rc < 0) - goto no_memory; + char *bootserver = virSocketFormatAddr(&network->def->bootserver); - APPEND_ARG(*argv, i++, "--dhcp-boot"); - APPEND_ARG_LIT(*argv, i++, boot); + if (!bootserver) + goto cleanup; + virCommandAddArgFormat(cmd, "%s%s%s", + network->def->bootfile, ",,", bootserver); + VIR_FREE(bootserver); + } else { + virCommandAddArg(cmd, network->def->bootfile); + } } -#undef APPEND_ARG - - return 0; - - no_memory: - virReportOOMError(); - error: - if (*argv) { - for (i = 0; (*argv)[i]; i++) - VIR_FREE((*argv)[i]); - VIR_FREE(*argv); - } - return -1; + ret = 0; +cleanup: + VIR_FREE(bridgeaddr); + return ret; } static int dhcpStartDhcpDaemon(virNetworkObjPtr network) { - const char **argv; - char *pidfile; - int ret = -1, i, err; + virCommandPtr cmd = NULL; + char *pidfile = NULL; + int ret = -1, err; network->dnsmasqPid = -1; if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) { networkReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot start dhcp daemon without IPv4 address for server")); - return -1; + goto cleanup; } if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) { virReportSystemError(err, _("cannot create directory %s"), NETWORK_PID_DIR); - return -1; + goto cleanup; } if ((err = virFileMakePath(NETWORK_STATE_DIR)) != 0) { virReportSystemError(err, _("cannot create directory %s"), NETWORK_STATE_DIR); - return -1; + goto cleanup; } if (!(pidfile = virFilePid(NETWORK_PID_DIR, network->def->name))) { virReportOOMError(); - return -1; + goto cleanup; } - argv = NULL; - if (networkBuildDnsmasqArgv(network, pidfile, &argv) < 0) { + cmd = virCommandNew(DNSMASQ); + if (networkBuildDnsmasqArgv(network, pidfile, cmd) < 0) { VIR_FREE(pidfile); - return -1; + goto cleanup; } - if (virRun(argv, NULL) < 0) + if (virCommandRun(cmd, NULL) < 0) goto cleanup; /* - * There really is no race here - when dnsmasq daemonizes, - * its leader process stays around until its child has - * actually written its pidfile. So by time virRun exits - * it has waitpid'd and guaranteed the proess has started - * and written a pid + * There really is no race here - when dnsmasq daemonizes, its + * leader process stays around until its child has actually + * written its pidfile. So by time virCommandRun exits it has + * waitpid'd and guaranteed the proess has started and written a + * pid */ if (virFileReadPid(NETWORK_PID_DIR, network->def->name, @@ -656,13 +582,9 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network) goto cleanup; ret = 0; - cleanup: VIR_FREE(pidfile); - for (i = 0; argv[i]; i++) - VIR_FREE(argv[i]); - VIR_FREE(argv); - + virCommandFree(cmd); return ret; } -- 1.7.3.2
On 12/10/2010 12:02 PM, Laine Stump wrote:
This is pretty straightforward - even though dnsmasq gets daemonized and uses a pid file, those things are both handled by the dnsmasq binary itself. And libvirt doesn't need any of the output of the dnsmasq command either, so we just setup the args and call virRun(). Mainly it was just a (mostly) mechanical job of replacing the APPEND_ARG() macro (and some other *printfs()) with virCommandAddArg*(). --- src/network/bridge_driver.c | 238 +++++++++++++++---------------------------- 1 files changed, 80 insertions(+), 158 deletions(-)
- if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile) < 0) - goto no_memory; - APPEND_ARG_LIT(*argv, i++, pidfileArg); + virCommandAddArgPair(cmd, "--pid-file", pidfile);
This technically changes from one arg to two, but that should be a safe change.
- APPEND_ARG(*argv, i++, "--conf-file="); - APPEND_ARG(*argv, i++, ""); + /* *no* conf file */ + virCommandAddArg(cmd, "--conf-file="); + virCommandAddArg(cmd, "");
dnsmasq really requires "--conf-file=" "" as two arguments? Yuck. Generally, it's either "--conf-file" "" with no equal, or just "--conf-file=" as a single argument, when one is being explicit that the argument to --conf-file is the empty string. But this is an identity transformation of the old code, so if it's a bug, it is pre-existing and worth fixing in a separate patch than this mechanical transformation.
if (network->def->tftproot) { - APPEND_ARG(*argv, i++, "--enable-tftp"); - APPEND_ARG(*argv, i++, "--tftp-root"); - APPEND_ARG(*argv, i++, network->def->tftproot); + virCommandAddArg(cmd, "--enable-tftp"); + virCommandAddArg(cmd, "--tftp-root"); + virCommandAddArg(cmd, network->def->tftproot);
If you want to compress the code even further, you can do things like: virCommandAddArgList(cmd, "--enable-tftp", "--tftp-root", network->def->tftproot, NULL); in a few places where you have back-to-back simple string additions. But it doesn't affect correctness, so you don't have to make that change. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/10/2010 01:34 PM, Eric Blake wrote:
On 12/10/2010 12:02 PM, Laine Stump wrote:
This is pretty straightforward - even though dnsmasq gets daemonized and uses a pid file, those things are both handled by the dnsmasq binary itself. And libvirt doesn't need any of the output of the dnsmasq command either, so we just setup the args and call virRun(). Mainly it was just a (mostly) mechanical job of replacing the APPEND_ARG() macro (and some other *printfs()) with virCommandAddArg*(). --- src/network/bridge_driver.c | 238 +++++++++++++++---------------------------- 1 files changed, 80 insertions(+), 158 deletions(-)
- if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile) < 0) - goto no_memory; - APPEND_ARG_LIT(*argv, i++, pidfileArg); + virCommandAddArgPair(cmd, "--pid-file", pidfile);
This technically changes from one arg to two, but that should be a safe change.
Oops - I didn't read the source. AddArgPair results in a single argument, %s=%s, and not two arguments. So my claim that it changed args from one to two was bogus. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/10/2010 03:34 PM, Eric Blake wrote:
On 12/10/2010 12:02 PM, Laine Stump wrote:
This is pretty straightforward - even though dnsmasq gets daemonized and uses a pid file, those things are both handled by the dnsmasq binary itself. And libvirt doesn't need any of the output of the dnsmasq command either, so we just setup the args and call virRun(). Mainly it was just a (mostly) mechanical job of replacing the APPEND_ARG() macro (and some other *printfs()) with virCommandAddArg*(). --- src/network/bridge_driver.c | 238 +++++++++++++++---------------------------- 1 files changed, 80 insertions(+), 158 deletions(-)
- if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile)< 0) - goto no_memory; - APPEND_ARG_LIT(*argv, i++, pidfileArg); + virCommandAddArgPair(cmd, "--pid-file", pidfile); This technically changes from one arg to two, but that should be a safe change.
How's that? virCommandAddArgPair does a single call to virCommandAddArgFormat(cmd, "%s=%s", ...)
- APPEND_ARG(*argv, i++, "--conf-file="); - APPEND_ARG(*argv, i++, ""); + /* *no* conf file */ + virCommandAddArg(cmd, "--conf-file="); + virCommandAddArg(cmd, "");
dnsmasq really requires "--conf-file=" "" as two arguments? Yuck.
I'm not sure. I'm just carrying forward what's already there. Seems pretty ugly to me too, but something like that seems too odd to assume it's just be an oversight.
Generally, it's either "--conf-file" "" with no equal, or just "--conf-file=" as a single argument, when one is being explicit that the argument to --conf-file is the empty string. But this is an identity transformation of the old code, so if it's a bug, it is pre-existing and worth fixing in a separate patch than this mechanical transformation.
if (network->def->tftproot) { - APPEND_ARG(*argv, i++, "--enable-tftp"); - APPEND_ARG(*argv, i++, "--tftp-root"); - APPEND_ARG(*argv, i++, network->def->tftproot); + virCommandAddArg(cmd, "--enable-tftp"); + virCommandAddArg(cmd, "--tftp-root"); + virCommandAddArg(cmd, network->def->tftproot);
If you want to compress the code even further, you can do things like:
virCommandAddArgList(cmd, "--enable-tftp", "--tftp-root", network->def->tftproot, NULL);
in a few places where you have back-to-back simple string additions.
Right. I didn't think of that. That seems harmless enough to do without a v2, so I'm squashing in the patch attached below.
But it doesn't affect correctness, so you don't have to make that change.
ACK
Pushed. Thanks!
On Fri, Dec 10, 2010 at 01:34:15PM -0700, Eric Blake wrote:
On 12/10/2010 12:02 PM, Laine Stump wrote:
This is pretty straightforward - even though dnsmasq gets daemonized and uses a pid file, those things are both handled by the dnsmasq binary itself. And libvirt doesn't need any of the output of the dnsmasq command either, so we just setup the args and call virRun(). Mainly it was just a (mostly) mechanical job of replacing the APPEND_ARG() macro (and some other *printfs()) with virCommandAddArg*(). --- src/network/bridge_driver.c | 238 +++++++++++++++---------------------------- 1 files changed, 80 insertions(+), 158 deletions(-)
- if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile) < 0) - goto no_memory; - APPEND_ARG_LIT(*argv, i++, pidfileArg); + virCommandAddArgPair(cmd, "--pid-file", pidfile);
This technically changes from one arg to two, but that should be a safe change.
- APPEND_ARG(*argv, i++, "--conf-file="); - APPEND_ARG(*argv, i++, ""); + /* *no* conf file */ + virCommandAddArg(cmd, "--conf-file="); + virCommandAddArg(cmd, "");
dnsmasq really requires "--conf-file=" "" as two arguments? Yuck. Generally, it's either "--conf-file" "" with no equal, or just "--conf-file=" as a single argument, when one is being explicit that the argument to --conf-file is the empty string. But this is an identity transformation of the old code, so if it's a bug, it is pre-existing and worth fixing in a separate patch than this mechanical transformation.
I'm fairly sure that dnsmsq does require it the way I did it originally. I remember finding its arg handling a little odd when I wrote this :-) Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump