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

[Eric Blake]

On 07/18/2011 01:54 AM, Osier Yang wrote:
> If the domain has managed state file, and --managed-state is
> not specified, then it fails with an error telling the user
> that a managed save image still exists.

Commit message is now out of date with the code, where you renamed 
things --managed-save.

>
> If the domain has managed state file, and --managed-state is
> specified, it invokes virDomainUndefineFlags. If
> virDomainUndefineFlags fails, then it tries to remove the managed
> save image using virDomainManagedSaveRemove first, with
> invoking virDomainUndefine following. (For compatibility between
> new virsh with this patch and older libvirt without this patch).
>
> Similarly if the domain has no managed state. See the codes for
> detail.
>
> NOTE: Have not removing the codes checking if the domain is running
> in function "cmdUndefine", it will go along with qemu driver's fix
> (allow to undefine a running domain).

Makes sense; after all, that's a separate fix.

> @@ -1447,11 +1458,50 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>           virDomainFree(dom);
>           return false;
>       }
> +
>       if (!(dom = vshCommandOptDomainBy(ctl, cmd,&name,
>                                         VSH_BYNAME|VSH_BYUUID)))
>           return false;
>
> -    if (virDomainUndefine(dom) == 0) {
> +    has_managed_save = virDomainHasManagedSaveImage(dom, 0);
> +    if (has_managed_save<  0) {
> +        virDomainFree(dom);
> +        return false;
> +    }

Not quite right.  If last_error->code == VIR_ERR_NO_SUPPORT, then the 
reason that virDomainHasManagedSaveImage returned -1 is because the 
hypervisor _can't_ create managed save, and therefore, we should not 
fail the undefine (because the undefine by definition won't have any 
managed state to worry about).

This logic needs to be updated to return false only if last_error->code 
is something besides VIR_ERR_NO_SUPPORT (meaning we hit a real error on 
a hypervisor that supports managed save, but can't answer us, rather 
than a hypervisor that doesn't have managed save in the first place).

> +
> +    if (flags == -1) {
> +        if (has_managed_save == 1) {
> +            vshError(ctl,
> +                     _("Refusing to undefine while domain managed save "
> +                       "image exists"));
> +            virDomainFree(dom);
> +            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) {
> +            virErrorPtr err = virGetLastError();

Overkill; look at how cmdVcpucount uses the virsh.c global last_error, 
rather than having to call virGetLastError().  That is, virsh already 
hooked up a custom handler that captures all errors at the point they 
are created, so you can then query last_error, rather than having to 
make more virGetLastError API calls yourself.

> +++ b/tools/virsh.pod
> @@ -807,10 +807,18 @@ hypervisor.
>   Output the device used for the TTY console of the domain. If the information
>   is not available the processes will provide an exit code of 1.
>
> -=item B<undefine>  I<domain-id>
> +=item B<undefine>  I<domain-id>  optional I<--managed-save>

Now that my virsh.pod cleanup is in (commit 08d3b0a2), this needs to be:

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


>
> -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>.
> +Undefine a domain. If the domain is running, this converts it to a
> +transient domain, without stopping it. If the domain is inactive,
> +the domain configuration is removed.
> +
> +The I<--managed-save>  flag guarantees that any managed save image(see
> +the B<managedsave>  command) is also cleaned up.  Without the flag, attempts
> +to undefine a domain with managed save image will fail.

s/with managed/with a managed/

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