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

[Laine Stump]

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!

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Combine-multiple-consecutive-virCommandAddArg-calls-.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101210/0166d736/attachment-0001.ksh>