[libvirt] [PATCH RFC] virsh: Add option to undefine storage with domains
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 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. Thanks for your comments (and time) :) 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")}, {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")); + 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
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
On 07/28/2011 04:10 PM, Dave Allan 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? Yes, managed images. 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
On Thu, Jul 28, 2011 at 03:41:01PM +0200, Peter Krempa wrote: pretty straight passthrough to the API. I'm sure others will have an opinion on that question as well. Well, yes, I found that out while looking for an interactive query method which I didn't find.
Other option, that comes into my mind is to add a command to list storage volumes for domains and then let the user specify volumes to be removed as parameters to the command "undefine".
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".
That definitely looks better. Thanks.
{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?
Definitely yes. It's kind of hard to see in the patch, because the contexts got chosen very badly, but the user may specify both together and gets expected results.
+ 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"));
On 07/29/2011 02:30 AM, Peter Krempa wrote:
Well, yes, I found that out while looking for an interactive query method which I didn't find.
I'm not opposed to an interactive query, but it does indeed put us into new territory for virsh, and we still have to think about scriptable non-interactive solutions.
Other option, that comes into my mind is to add a command to list storage volumes for domains
We need to do that anyways. It would be helpful in a lot of other contexts, such as my work on snapshots. I think you have a good idea for improving things, but it needs more polish, and is definitely post-0.9.4 material. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Dave Allan -
Eric Blake -
Peter Krempa