[libvirt] [PATCH 3/5] virsh-domain: Add --live, --config, --current logic to cmdDetachInterface

[Peter Krempa]

On 03/27/13 13:53, Martin Kletzander wrote:
> On 03/21/2013 05:42 PM, Peter Krempa wrote:
>> Use the established approach to improve this function too.
>> ---
>>   tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
>>   tools/virsh.pod      | 15 +++++++++++----
>>   2 files changed, 50 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 6741837..df72c78 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = {
>>        .help = N_("MAC address")
>>       },
>>       {.name = "persistent",
>> -     .type = VSH_OT_ALIAS,
>> -     .help = "config"
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("make live change persistent")
>>       },
>>       {.name = "config",
>>        .type = VSH_OT_BOOL,
>>        .help = N_("affect next boot")
>>       },
>> +    {.name = "live",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect running domain")
>> +    },
>> +    {.name = "current",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect current domain")
>> +    },
>> +    {.name = "force",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("force device update")
>> +    },
>
> Adding non-used --force paramter, copy-paste error?
>
>>       {.name = NULL}
>>   };
>>
>> @@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>>       xmlNodePtr cur = NULL, matchNode = NULL;
>>       xmlBufferPtr xml_buf = NULL;
>>       const char *mac =NULL, *type = NULL;
>> -    char *doc;
>> +    char *doc = NULL;
>>       char buf[64];
>>       int i = 0, diff_mac;
>>       int ret;
>>       int functionReturn = false;
>> -    unsigned int flags;
>> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>> +    bool current = vshCommandOptBool(cmd, "current");
>> +    bool config = vshCommandOptBool(cmd, "config");
>> +    bool live = vshCommandOptBool(cmd, "live");
>> +    bool persistent = vshCommandOptBool(cmd, "persistent");
>> +
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);
>
> Same as in previous patch, remove this line.
>
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
>> +
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
>> +
>> +    if (config || persistent)
>> +        flags |= VIR_DOMAIN_AFFECT_CONFIG;
>> +    if (live)
>> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
>>
>>       if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>>           return false;
>> @@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>>       if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
>>           goto cleanup;
>>
>> -    doc = virDomainGetXMLDesc(dom, 0);
>> -    if (!doc)
>> +    if (persistent &&
>> +        virDomainIsActive(dom) == 1)
>> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
>> +
>> +    if (!(doc = virDomainGetXMLDesc(dom, 0)))
>>           goto cleanup;
>>
>> -    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
>> -    VIR_FREE(doc);
>> -    if (!xml) {
>> +    if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) {
>>           vshError(ctl, "%s", _("Failed to get interface information"));
>>           goto cleanup;
>>       }
>> @@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>>           goto cleanup;
>>       }
>>
>> -    if (vshCommandOptBool(cmd, "config")) {
>> -        flags = VIR_DOMAIN_AFFECT_CONFIG;
>> -        if (virDomainIsActive(dom) == 1)
>> -            flags |= VIR_DOMAIN_AFFECT_LIVE;
>> +    if (flags != 0) {
>>           ret = virDomainDetachDeviceFlags(dom,
>>                                            (char *)xmlBufferContent(xml_buf),
>>                                            flags);
>
> This if seems pointless, because we usually call the *Flags() function
> with flags=0, but this particular function is (for example in qemu
> driver) calling the *Flags() function with AFFECT_LIVE.  The current
> virsh code also adds the AFFECT_LIVE automatically, as you can see...
>
>> @@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>>       }
>>
>>    cleanup:
>> +    VIR_FREE(doc);
>
> If this fixes a leak, it could be mentioned in the commit message.
>
>>       virDomainFree(dom);
>>       xmlXPathFreeObject(obj);
>>       xmlXPathFreeContext(ctxt);
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index a9e8c65..ebbe201 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent configuration, effect observed
>>   on next boot, for compatibility purposes, I<--persistent> is alias of
>>   I<--config>.
>>
>> -=item B<detach-interface> I<domain> I<type> [I<--mac mac>] [I<--config>]
>> +=item B<detach-interface> I<domain> I<type> [I<--mac mac>]
>> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
>>
>>   Detach a network interface from a domain.
>>   I<type> can be either I<network> to indicate a physical network device or
>>   I<bridge> to indicate a bridge to a device. It is recommended to use the
>>   I<mac> option to distinguish between the interfaces if more than one are
>>   present on the domain.
>> -If I<--config> is specified, alter persistent configuration, effect observed
>> -on next boot, for compatibility purposes, I<--persistent> is alias of
>> -I<--config>.
>> +
>> +If I<--live> is specified, affect a running domain.
>> +If I<--config> is specified, affect the next startup of a persistent domain.
>> +If I<--current> is specified, affect the current domain state.
>> +Both I<--live> and I<--config> flags may be given, but I<--current> is
>> +exclusive. Not specifying any flag is the same as specifying I<--current>.

I changed the last sentence to: When no flag is specified legacy API is 
used whose behavior depends on the hypervisor driver.

Peter