On 07/28/2011 04:10 PM, Dave Allan wrote:
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?
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
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"));
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
