[libvirt] [PATCH 8/8] UndefineFlags: Extend virsh undefine to support new flag

[Eric Blake]

On 07/13/2011 04:19 AM, Osier Yang wrote:
> ---
>  tools/virsh.c   |   12 +++++++++++-
>  tools/virsh.pod |    6 +++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 3cdf043..f81e923 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")},
> +    {"undefine-managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")},

Dave suggested '--remove-managed-state', but given the name of our
existing virsh command:

managedsave-remove

I'd feel slightly better with:

virsh undefine domain --managed-save

[Meanwhile, we don't have any virsh command that directly exposes
virDomainHasManagedSaveImage - that information should probably be added
as a flag to an existing command, perhaps 'virsh list --managed-save'
adding a column on whether a managed save image exists for each domain.]

> +    int flags = 0;
> +    int undefine_managed_state = vshCommandOptBool(cmd, "undefine-managed-state");
> +
> +    if (undefine_managed_state)
> +        flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE;
> +
> +    if (!undefine_managed_state)
> +        flags = -1;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return false;
> @@ -1440,7 +1449,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>                                        VSH_BYNAME|VSH_BYUUID)))
>          return false;
>  
> -    if (virDomainUndefine(dom) == 0) {
> +    if (((flags == -1) ? virDomainUndefine(dom) :
> +        virDomainUndefineWithFlags(dom, flags)) == 0) {

Most times when we add a new flag to virsh, we can't support it without
using the new API.  But here, we can, and so I think we should.  That
is, if you virsh 0.9.4 as the client, and are talking to two different
servers (one 0.9.3 and one 0.9.4), you get the following behavior for a
domain that has a managed save image:

server 0.9.3:
virsh undefine dom -> call virDomainHasManagedSaveImage - if that is
true, then refuse to run virDomainUndefine (thus papering over the 0.9.3
safety bug).  If false, then virDomainUndefine is safe.
virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which
fails with unimplemented.  Falls back to combo
virDomainManagedSaveRemove/virDomainUndefine, which does the job we want.

server 0.9.4
virsh undefine dom -> call virDomainHasManagedSaveImage - if that is
true, then refuse to run virDomainUndefine.  If false, then
virDomainUndefine is safe.
virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which
succeeds and does the job we want (or fails, but with an error different
than unimplemented)

an alternative would be that calling 'virsh undefine dom' without flags
skips virDomainHasManagedSaveImage, and just blindly calls
virDomainUndefine - in that case, a server at 0.9.4 would properly fail,
but a server at 0.9.3 would still expose the bug that we are trying to
avoid of leaving stale managed state behind.


For precedence in making virsh try a fallback to older API when the new
API is unsupported, see the setvcpus command.

> +++ b/tools/virsh.pod
> @@ -804,11 +804,15 @@ 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<--undefine-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>.

Oh, this is different than libvirt.c, which claimed you can run
'undefine' on a running persistent domain.  Which is it?  Can undefine
make a running domain transient, or must it be on an inactive domain?

>  
> +If I<--undefine-managed-state> is specified, the managed state file will
> +be removed along with the domain undefine peocess, the entire domain
> +undefine process will fail if it fails on removing the managed state file.

Given my above thoughts (and once I validate the behavior of undefine on
a running domain), I'd word this as:

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

Undefine the configuration for a domain.  If domain is not running, the
domain name or UUID must be used as the I<domain-id>; if the domain is
running, then this converts a persistent domain to a transient domain.

The I<--managed-save> flag guarantees that any managed state (see the
B<managesave> command) is also cleaned up.  Without the flag, attempts
to undefine a domain with managed state will fail.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110713/c2f5e136/attachment-0001.sig>