[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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"));
> +            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 redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]