[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