On Thu, Jul 28, 2011 at 03:41:01PM +0200, Peter Krempa wrote:
Adds an option to virsh undefine command to undefine managed
storage volumes along with (inactive) domains. Storage volumes
are enumerated and the user may interactivly choose volumes
to delete.
Unmanaged volumes are listed and the user shall delete them
manualy.
---
I marked this as a RFC because I am concerned about my "naming scheme" of the
added parameters.
I couldn't decide which of the following "volumes/storage/disks/..." to
use. I'd appreciate your
comments on this.
This is my second approach to this problem after I got some really good critique from
Eric,
Daniel and Dave. The user has the choice to activate an interactive mode, that allows to
select on a
per-device basis which volumes/disks to remove along with the domain.
To avoid possible problems, I only allowed to remove storage for inactive domains and
unmanaged
I think you mean managed images, right?
images (which sidetracked me a lot on my previous attempt) are left
to a action of the user.
(the user is notified about any unmanaged image for the domain).
My next concern is about interactive of the user. I tried to implement a boolean query
function,
but I'd like to know if I took the right path, as I couldn't find any example in
virsh from which I
could learn.
We haven't previously implemented that much interactivity in virsh,
and I'm not sure I want to go down that road. virsh is generally a
pretty straight passthrough to the API. I'm sure others will have an
opinion on that question as well.
Thanks for your comments (and time) :)
A few comments inline.
Dave
Peter Krempa
tools/virsh.c | 265 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 259 insertions(+), 6 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 61f69f0..3795d2b 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -295,6 +295,9 @@ static int vshCommandOptULongLong(const vshCmd *cmd, const char
*name,
static bool vshCommandOptBool(const vshCmd *cmd, const char *name);
static const vshCmdOpt *vshCommandOptArgv(const vshCmd *cmd,
const vshCmdOpt *opt);
+static int vshInteractiveBoolPrompt(vshControl *ctl,
+ const char *prompt,
+ bool *confirm);
#define VSH_BYID (1 << 1)
#define VSH_BYUUID (1 << 2)
@@ -1422,6 +1425,8 @@ static const vshCmdInfo info_undefine[] = {
static const vshCmdOptDef opts_undefine[] = {
{"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or
uuid")},
{"managed-save", VSH_OT_BOOL, 0, N_("remove domain managed state
file")},
+ {"disks", VSH_OT_BOOL, 0, N_("remove associated disk images managed
in storage pools (interactive)")},
+ {"disks-all", VSH_OT_BOOL, 0, N_("remove all associated disk images
managed in storage pools")},
I think it would be clearer stated as "remove all associated storage
volumes", and correspondingly, call the option "storage-volumes".
{NULL, 0, 0, NULL}
};
@@ -1434,9 +1439,25 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
int id;
int flags = 0;
int managed_save = vshCommandOptBool(cmd, "managed-save");
+ int remove_disks = vshCommandOptBool(cmd, "disks");
+ int remove_all_disks = vshCommandOptBool(cmd, "disks-all");
int has_managed_save = 0;
int rc = -1;
+ char *domxml;
+ xmlDocPtr xml = NULL;
+ xmlXPathContextPtr ctxt = NULL;
+ xmlXPathObjectPtr obj = NULL;
+ xmlNodePtr cur = NULL;
+ int i = 0;
+ char *source = NULL;
+ char *target = NULL;
+ char *type = NULL;
+ xmlBufferPtr xml_buf = NULL;
+ virStorageVolPtr volume = NULL;
+ int state;
+ bool confirm = false;
+
if (managed_save)
flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -1475,15 +1496,172 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
}
}
- if (flags == -1) {
- if (has_managed_save == 1) {
+
+ if (flags == -1 && has_managed_save == 1) {
+ vshError(ctl,
+ _("Refusing to undefine while domain managed save "
+ "image exists"));
How does this interact with --managed-save? Can a user specify both
--managed-save and --disks to remove both managed save and storage volumes?
+ virDomainFree(dom);
+ return false;
+ }
+
+ if (remove_disks || remove_all_disks) {
+ if ((state = vshDomainState(ctl, dom, NULL)) < 0) {
+ vshError(ctl, _("Failed to get domain state"));
+ goto disk_error;
+ }
+
+ /* removal of storage is possible only for inactive domains */
+ if (!((state == VIR_DOMAIN_SHUTOFF) ||
+ (state == VIR_DOMAIN_CRASHED))) {
vshError(ctl,
- _("Refusing to undefine while domain managed save "
- "image exists"));
- virDomainFree(dom);
- return false;
+ _("Domain needs to be inactive to delete it with associated
storage"));
+ goto disk_error;
+ }
+
+ if (remove_disks && !ctl->imode) {
+ vshError(ctl, "%s\n", _("Option --disks is available only in
interactive mode"));
+ goto disk_error;
+ }
+
+ domxml = virDomainGetXMLDesc(dom, 0);
+ if (!domxml) {
+ vshError(ctl, "%s", _("Failed to get disk
information"));
+ goto disk_error;
+ }
+
+ xml = xmlReadDoc((const xmlChar *) domxml, "domain.xml", NULL,
+ XML_PARSE_NOENT |
+ XML_PARSE_NONET |
+ XML_PARSE_NOWARNING);
+ VIR_FREE(domxml);
+
+ if (!xml) {
+ vshError(ctl, "%s", _("Failed to get disk
information"));
+ goto disk_error;
+ }
+
+ ctxt = xmlXPathNewContext(xml);
+ if (!ctxt) {
+ vshError(ctl, "%s", _("Failed to get disk
information"));
+ goto disk_error;
+ }
+
+ obj = xmlXPathEval(BAD_CAST "/domain/devices/disk", ctxt);
+ if ((obj == NULL) || (obj->type != XPATH_NODESET) ||
+ (obj->nodesetval == NULL)) {
+ vshError(ctl, "%s", _("Failed to get disk
information"));
+ goto disk_error;
+ }
+
+ for (i = 0; i < obj->nodesetval->nodeNr; i++) {
+ cur = obj->nodesetval->nodeTab[i]->children;
+
+ type = virXMLPropString(cur, "device");
+
+ while (cur != NULL) {
+ if (cur->type == XML_ELEMENT_NODE) {
+ if (xmlStrEqual(cur->name, BAD_CAST "target"))
+ target = virXMLPropString(cur, "dev");
+ else if (xmlStrEqual(cur->name, BAD_CAST "source"))
+ source = virXMLPropString(cur, "file");
+ }
+ cur = cur->next;
+ }
+
+ if (!source) {
+ VIR_FREE(target);
+ VIR_FREE(type);
+ }
+
+ volume = virStorageVolLookupByPath(ctl->conn, (const char *) source);
+ if (!volume) {
+ vshPrint(ctl, "%s %s %s %s\n",
+ _("Volume: Source:"), (const char *)source,
+ _("Target:"), (const char *) target);
+ vshPrint(ctl, _("This volume isn't managed by any storage pool,
"
+ "please delete it manualy\n\n"));
+ /* remove error indication */
+ virFreeError(last_error);
+ last_error = NULL;
+ } else {
+ vshPrint(ctl, "%s %s %s %s\n",
+ _("Volume: Source:"), (const char *)source,
+ _("Target:"), (const char *) target);
+
+ if (remove_all_disks) {
+ confirm = true;
+ } else {
+ if (vshInteractiveBoolPrompt(ctl,
+ _("Do you want to undefine this
volume?"),
+ &confirm) < 0) {
+ vshError(ctl, _("\nError while geting response from
user"));
+ virStorageVolFree(volume);
+ goto disk_error;
+ }
+ }
+
+ /* removal of volume */
+ if (confirm) {
+ if (virStorageVolDelete(volume, 0) == 0) {
+ virStorageVolFree(volume);
+
+ vshPrint(ctl, _("Volume deleted\n\n"));
+
+ /* remove definition of volume from xml */
+ xml_buf = xmlBufferCreate();
+ if (!xml_buf) {
+ vshPrint(ctl, _("Failed to create XML buffer. "
+ "If domain undefinition fails, domain
will be left in inconsistent state.\n\n"));
+ goto disk_next;
+ }
+
+ if (xmlNodeDump(xml_buf, xml, obj->nodesetval->nodeTab[i],
0, 0) < 0) {
+ vshPrint(ctl, _("Failed to extract XML volume
description. "
+ "If domain undefinition fails, domain
will be left in inconsistent state.\n\n"));
+
+ xmlBufferFree(xml_buf);
+ xml_buf = NULL;
+ goto disk_next;
+ }
+
+ if (virDomainDetachDeviceFlags(dom,
+ (char *)
xmlBufferContent(xml_buf),
+ VIR_DOMAIN_AFFECT_CONFIG) < 0)
{
+ vshPrint(ctl,
+ _("Failed to remove volume \"%s\"
from configuration. "
+ "If domain undefinition fails, domain will
be left in inconsistent state.\n\n"),
+ source);
+
+ xmlBufferFree(xml_buf);
+ xml_buf = NULL;
+ goto disk_next;
+ }
+
+ xmlBufferFree(xml_buf);
+ xml_buf = NULL;
+
+ } else {
+ virStorageVolFree(volume);
+
+ vshError(ctl, _("Failed to delete volume."));
+ goto disk_error;
+ }
+ }
+ }
+
+disk_next:
+ VIR_FREE(source);
+ VIR_FREE(target);
+ VIR_FREE(type);
}
+ xmlXPathFreeObject(obj);
+ xmlXPathFreeContext(ctxt);
+ xmlFreeDoc(xml);
+ } /* end of disk undefine stuff */
+
+ if (flags == -1) {
rc = virDomainUndefine(dom);
} else {
rc = virDomainUndefineFlags(dom, flags);
@@ -1520,6 +1698,21 @@ end:
virDomainFree(dom);
return ret;
+
+disk_error:
+ VIR_FREE(source);
+ VIR_FREE(target);
+ VIR_FREE(type);
+
+ xmlXPathFreeObject(obj);
+ xmlXPathFreeContext(ctxt);
+ xmlFreeDoc(xml);
+ xmlBufferFree(xml_buf);
+
+ virDomainFree(dom);
+
+ vshError(ctl, _("\nFailed to undefine domain %s"), name);
+ return false;
}
@@ -14736,6 +14929,66 @@ vshReadline (vshControl *ctl, const char *prompt)
#endif /* !USE_READLINE */
+
+/*
+ * Propmpt for boolean question (yes/no)
+ *
+ * Returns "true" on positive answer, "false" on negative
+ * -1 on error.
+ */
+static int
+vshInteractiveBoolPrompt(vshControl *ctl,
+ const char *prompt,
+ bool *confirm) {
+ const char *positive = _("yes");
+ const char *negative = _("no");
+ char *r;
+ char buff[100];
+ int ret = -1;
+ int len;
+ int c;
+
+ if (confirm == NULL)
+ return ret;
+
+ if (!ctl->imode)
+ return ret;
+
+ while (ret == -1) {
+ vshPrint(ctl, "%s (%s/%s)? ", prompt, positive, negative);
+
+ r = fgets(buff, sizeof(buff), stdin);
+ if (r == NULL)
+ break;
+ len = strlen(buff);
+ if (len > 0 && buff[len-1] == '\n')
+ buff[len-1] = '\0';
+ else
+ /* eat rest of line */
+ while ((c = fgetc(stdin) != EOF))
+ if (c == '\n')
+ break;
+
+ /* compare */
+ if (STRCASEEQLEN(buff, positive, strlen(positive))) {
+ ret = 0;
+ *confirm = true;
+ break;
+ }
+
+ if (STRCASEEQLEN(buff, negative, strlen(negative))) {
+ ret = 0;
+ *confirm = false;
+ break;
+ }
+
+ /* errorneus response - warn and ask again*/
+ vshPrint(ctl, "\"%s\" %s\n", buff, _("Response not
understood"));
+
+ }
+ return ret;
+}
+
/*
* Deinitialize virsh
*/
--
1.7.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list