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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list