Re: [libvirt] [PATCH] Convert dhcpStartDhcpDaemon from virRun to virCommand

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