[libvirt] [PATCH RFC] virsh: Add option to undefine storage with domains
Peter Krempa
pkrempa at redhat.com
Fri Jul 29 08:30:28 UTC 2011
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"));
More information about the libvir-list
mailing list