[libvirt] [PATCH v2 7/8] undefine: Extend virsh undefine to support the new API

[Eric Blake]

On 07/15/2011 03:06 AM, Osier Yang wrote:
> If the domain has managed state file, and --managed-state is
> not specified, then it fails with error prompt to tell user
> there is managed state file exists.

Grammar suggestion:

then it fails with an error telling the user that a managed save still
exists.


Hmm, I'm now having second thoughts about the names
"VIR_DOMAIN_UNDEFINE_MANAGED_STATE" and "--managed-state", since the
name of the API is virDomainManagedSave and the virsh command is
managedsave.  Would it be better to go with:

VIR_DOMAIN_UNDEFINE_MANAGED_SAVE and --managed-save?  This would mean
tweaking the earlier patches in this series.

> 
> And the domain has managed state file, and --managed-state is

s/And/If/

> specified, it invokes virDomainUndefineFlags, if
> virDomainUndefineFlag fails, then it trys to remove the managed

s/trys/tries/

> state file using virDomainManagedSaveRemove first, with
> invoking virDomainUndefine following. (For compatibility between
> new virsh with this patch and older libvirt without this fix)
> 
> Simliar if the domain has no managed state. See the codes for

s/Simliar/Similarly/

> detail.
> ---
>  tools/virsh.c   |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  tools/virsh.pod |    6 +++++-
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 4af8fea..8a62612 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = {
>  
>  static const vshCmdOptDef opts_undefine[] = {
>      {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")},
> +    {"managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")},

s/state/save/g, given my above thoughts.

>      {NULL, 0, 0, NULL}
>  };
>  
> @@ -1419,6 +1420,16 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>      bool ret = true;
>      const char *name = NULL;
>      int id;
> +    int flags = 0;
> +    int managed_state = vshCommandOptBool(cmd, "managed-state");
> +    int has_managed_state = 0;
> +    int rc = -1;
> +
> +    if (managed_state)
> +        flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE;
> +
> +    if (!managed_state)
> +        flags = -1;

I'm not sure if you need this line.  Instead...

>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return false;
> @@ -1440,7 +1451,37 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>                                        VSH_BYNAME|VSH_BYUUID)))
>          return false;
>  
> -    if (virDomainUndefine(dom) == 0) {
> +    has_managed_state = virDomainHasManagedSaveImage(dom, 0);
> +    if (has_managed_state < 0)
> +        return false;
> +
> +    if (flags == -1) {

...this conditional can just be if (!managed_state)

> +        if (has_managed_state == 1) {
> +            vshError(ctl,
> +                     _("Refusing to undefine with managed state "
> +                       "file exists"));

Grammar:

Refusing to undefine while managed save image exists

> +            return false;
> +        }
> +        
> +        rc = virDomainUndefine(dom);
> +    } else {
> +        rc = virDomainUndefineFlags(dom, flags);
> +
> +        /* It might fail for virDomainUndefineFlags is not
> +         * supported on older libvirt, try to undefine the
> +         * domain with combo virDomainManagedSaveRemove and
> +         * virDomainUndefine.
> +         */
> +        if (rc < 0) {

Here, we should optimize by checking the error.  If the error is
VIR_ERR_NO_SUPPORT, then we go with the fallback; but if it is anything
else, then virDomainUndefineFlags exists but failed, and the fallback
would also fail, so give up now.

> +            if (has_managed_state && 
> +                virDomainManagedSaveRemove(dom, 0) < 0)
> +                return false;

This early return...

> +
> +            rc = virDomainUndefine(dom);
> +        }
> +    }
> +
> +    if (rc == 0) {
>          vshPrint(ctl, _("Domain %s has been undefined\n"), name);
>      } else {
>          vshError(ctl, _("Failed to undefine domain %s"), name);

...will lose out on this error message.

>  
> -=item B<undefine> I<domain-id>
> +=item B<undefine> I<domain-id> I<--managed-state>

managed-state (or managed-save, whatever we name it) is optional, so
this should be:

=item B<undefine> I<domain-id> [I<--managed-state>]

>  
>  Undefine the configuration for an inactive domain. Since it's not running
>  the domain name or UUID must be used as the I<domain-id>.
>  
> +The I<--managed-save> flag guarantees that any managed state (see the
> +B<managesave> command) is also cleaned up.  Without the flag, attempts

s/managesave/managedsave/

> +to undefine a domain with managed state will fail.
> +
>  =item B<vcpucount> I<domain-id>  optional I<--maximum> I<--current>
>  I<--config> I<--live>
>  

Probably needs a v3.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org