[libvirt] [PATCH RFC] virsh: Add option to undefine storage with domains
Dave Allan
dallan at redhat.com
Thu Jul 28 14:10:21 UTC 2011
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list