Re: [libvirt] [PATCH RFC] virsh: Add option to undefine storage with domains

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"));