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!
>From 46b70312e064a748400b6d5afeeb040a941a07b3 Mon Sep 17 00:00:00 2001
From: Laine Stump <laine@xxxxxxxxx>
Date: Fri, 10 Dec 2010 15:49:37 -0500
Subject: [PATCH] Combine multiple consecutive virCommandAddArg calls into virCommandAddArgList
---
src/network/bridge_driver.c | 27 +++++++++++----------------
1 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 9802222..c3f32d7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -421,19 +421,15 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
* Needed to ensure dnsmasq uses same algorithm for processing
* multiple namedriver entries in /etc/resolv.conf as GLibC.
*/
- virCommandAddArg(cmd, "--strict-order");
- virCommandAddArg(cmd, "--bind-interfaces");
+ virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL);
- if (network->def->domain) {
- virCommandAddArg(cmd, "--domain");
- virCommandAddArg(cmd, network->def->domain);
- }
+ if (network->def->domain)
+ virCommandAddArgList(cmd, "--domain", network->def->domain, NULL);
virCommandAddArgPair(cmd, "--pid-file", pidfile);
/* *no* conf file */
- virCommandAddArg(cmd, "--conf-file=");
- virCommandAddArg(cmd, "");
+ virCommandAddArgList(cmd, "--conf-file=", "", NULL);
/*
* XXX does not actually work, due to some kind of
@@ -444,11 +440,10 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
* virCommandAddArg(cmd, "--interface");
* virCommandAddArg(cmd, network->def->bridge);
*/
- virCommandAddArg(cmd, "--listen-address");
- virCommandAddArg(cmd, bridgeaddr);
-
- virCommandAddArg(cmd, "--except-interface");
- virCommandAddArg(cmd, "lo");
+ virCommandAddArgList(cmd,
+ "--listen-address", bridgeaddr,
+ "--except-interface", "lo",
+ NULL);
for (r = 0 ; r < network->def->nranges ; r++) {
char *saddr = virSocketFormatAddr(&network->def->ranges[r].start);
@@ -500,9 +495,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
}
if (network->def->tftproot) {
- virCommandAddArg(cmd, "--enable-tftp");
- virCommandAddArg(cmd, "--tftp-root");
- virCommandAddArg(cmd, network->def->tftproot);
+ virCommandAddArgList(cmd, "--enable-tftp",
+ "--tftp-root", network->def->tftproot,
+ NULL);
}
if (network->def->bootfile) {
--
1.7.3.2
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list