Re: [PATCH 1/2] virsh: Introduce virshDomainDetachInterface function

[Cole Robinson <crobinso@xxxxxxxxxx>]

On 05/02/2016 09:59 AM, Nitesh Konkar wrote:
> virshDomainDetachInterface handles virsh interface
> detach from the specified live/config domain xml.
> 
> Signed-off-by: Nitesh Konkar <nitkon12@xxxxxxxxxxxxxxxxxx>
> ---
>  tools/virsh-domain.c | 104 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0a6caae..d9fde4d 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11199,39 +11199,22 @@ static const vshCmdOptDef opts_detach_interface[] = {
>  };
>  

>  static bool
> -cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> +virshDomainDetachInterface(char *doc,
> +                           unsigned int flags,
> +                           virDomainPtr dom,
> +                           vshControl *ctl,
> +                           const vshCmd *cmd)
>  {

The split is a bit wrong IMO. I don't think virshDomainDetachInterface should
be doing any command line processing. I think the args should be:

virshDomainDetachInterface(vshControl *ctl,
			   virDomainPtr dom,
                           unsigned int flags,
                           bool current,
                           const char *type,
                           const char *mac);

One other comment below:

> -    virDomainPtr dom = NULL;
>      xmlDocPtr xml = NULL;
>      xmlXPathObjectPtr obj = NULL;
>      xmlXPathContextPtr ctxt = NULL;
>      xmlNodePtr cur = NULL, matchNode = NULL;
> -    char *detach_xml = NULL;
>      const char *mac = NULL, *type = NULL;
> -    char *doc = NULL;
> -    char buf[64];
> -    int diff_mac;
> +    char *detach_xml = NULL, buf[64];
> +    bool current = vshCommandOptBool(cmd, "current");
> +    int diff_mac, ret;
>      size_t i;
> -    int ret;
>      bool functionReturn = false;
> -    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, 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 = virshCommandOptDomain(ctl, cmd, NULL)))
> -        return false;
>  
>      if (vshCommandOptStringReq(ctl, cmd, "type", &type) < 0)
>          goto cleanup;
> @@ -11239,18 +11222,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
>          goto cleanup;
>  
> -    if (persistent &&
> -        virDomainIsActive(dom) == 1)
> -        flags |= VIR_DOMAIN_AFFECT_LIVE;
> -
> -    if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> -        doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
> -    else
> -        doc = virDomainGetXMLDesc(dom, 0);
> -
> -    if (!doc)
> -        goto cleanup;
> -
>      if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) {
>          vshError(ctl, "%s", _("Failed to get interface information"));
>          goto cleanup;
> @@ -11315,20 +11286,67 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>      else
>          ret = virDomainDetachDevice(dom, detach_xml);
>  
> -    if (ret != 0) {
> +    if (ret == 0)
> +        functionReturn = true;
> +
> + cleanup:
> +    VIR_FREE(detach_xml);
> +    xmlFreeDoc(xml);
> +    xmlXPathFreeObject(obj);
> +    xmlXPathFreeContext(ctxt);
> +    return functionReturn;
> +}
> +
> +
> +static bool
> +cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom = NULL;
> +    char *doc = NULL;
> +    bool functionReturn = false;
> +    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, 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 = virshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if (persistent &&
> +        virDomainIsActive(dom) == 1)
> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> +        doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
> +    else
> +        doc = virDomainGetXMLDesc(dom, 0);
> +
> +    if (!doc)
> +        goto cleanup;
> +    else
> +       functionReturn = virshDomainDetachInterface(doc, flags, dom, ctl, cmd);
> +
> + cleanup:
> +    if (functionReturn == false) {
>          vshError(ctl, "%s", _("Failed to detach interface"));
>      } else {
>          vshPrint(ctl, "%s", _("Interface detached successfully\n"));
>          functionReturn = true;
>      }
>  
> - cleanup:
>      VIR_FREE(doc);
> -    VIR_FREE(detach_xml);
>      virDomainFree(dom);
> -    xmlXPathFreeObject(obj);
> -    xmlXPathFreeContext(ctxt);
> -    xmlFreeDoc(xml);
>      return functionReturn;

Drop functionReturn entirely here. set ret=-1 at init time, then return ret == 0;

Otherwise it looks good.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list