4:40 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org