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

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.