[libvirt] [PATCH]virsh: improve usability of '--print-xml' flag for attach-disk command

[Chen Hanxiao]

> -----Original Message-----
> From: Eric Blake [mailto:eblake at redhat.com]
> Sent: Friday, October 18, 2013 5:45 AM
> To: Chen Hanxiao; libvir-list at redhat.com
> Subject: Re: [libvirt] [PATCH]virsh: improve usability of '--print-xml' flag for
> attach-disk command
> 
> Just code motion - makes sense to delay probing for the domain until you
> know you need it.
> 
Wow

> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 2aed9f9..565966d 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -528,13 +528,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd
> *cmd)
> >      if (live)
> >          flags |= VIR_DOMAIN_AFFECT_LIVE;
> >
> > -    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> > -        return false;
> > -
> > -    if (persistent &&
> > -        virDomainIsActive(dom) == 1)
> > -        flags |= VIR_DOMAIN_AFFECT_LIVE;
> > -
> >      if (vshCommandOptStringReq(ctl, cmd, "source", &source) < 0 ||
> >          vshCommandOptStringReq(ctl, cmd, "target", &target) < 0 ||
> >          vshCommandOptStringReq(ctl, cmd, "driver", &driver) < 0 ||
> > @@ -672,6 +665,13 @@ cmdAttachDisk(vshControl *ctl, const vshCmd
> *cmd)
> >          goto cleanup;
> 
> Ouch - you can now go to cleanup while dom is still NULL.
> 
> >      }
> >
> > +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> > +        return false;
> 
> and this is wrong - if you get here, you've allocated resources that
> need the cleanup label.
> 
> > +
> > +    if (persistent &&
> > +        virDomainIsActive(dom) == 1)
> > +        flags |= VIR_DOMAIN_AFFECT_LIVE;
> > +
> >      if (flags)
> >          ret = virDomainAttachDeviceFlags(dom, xml, flags);
> >      else
> >
> 
> ACK with this squashed in, and pushed.

Thanks for your kindly help.

> 
> diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c
> index 45c4823..b75f331 100644
> --- i/tools/virsh-domain.c
> +++ w/tools/virsh-domain.c
> @@ -666,7 +666,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      }
> 
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> -        return false;
> +        goto cleanup;
> 
>      if (persistent &&
>          virDomainIsActive(dom) == 1)
> @@ -686,7 +686,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> 
>   cleanup:
>      VIR_FREE(xml);
> -    virDomainFree(dom);
> +    if (dom)
> +        virDomainFree(dom);
>      virBufferFreeAndReset(&buf);
>      return functionReturn;
>  }
> 
> 
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org