[libvirt] [PATCH v2 7/8] undefine: Extend virsh undefine to support the new API

[Osier Yang]

于 2011年07月16日 05:40, Eric Blake 写道:
> On 07/15/2011 03:06 AM, Osier Yang wrote:
>> If the domain has managed state file, and --managed-state is
>> not specified, then it fails with error prompt to tell user
>> there is managed state file exists.
> Grammar suggestion:
>
> then it fails with an error telling the user that a managed save still
> exists.
>
>
> Hmm, I'm now having second thoughts about the names
> "VIR_DOMAIN_UNDEFINE_MANAGED_STATE" and "--managed-state", since the
> name of the API is virDomainManagedSave and the virsh command is
> managedsave.  Would it be better to go with:
>
> VIR_DOMAIN_UNDEFINE_MANAGED_SAVE and --managed-save?  This would mean
> tweaking the earlier patches in this series.
>
>> And the domain has managed state file, and --managed-state is
> s/And/If/
>
>> specified, it invokes virDomainUndefineFlags, if
>> virDomainUndefineFlag fails, then it trys to remove the managed
> s/trys/tries/
>
>> state file using virDomainManagedSaveRemove first, with
>> invoking virDomainUndefine following. (For compatibility between
>> new virsh with this patch and older libvirt without this fix)
>>
>> Simliar if the domain has no managed state. See the codes for
> s/Simliar/Similarly/
>
>> detail.
>> ---
>>   tools/virsh.c   |   44 +++++++++++++++++++++++++++++++++++++++++++-
>>   tools/virsh.pod |    6 +++++-
>>   2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 4af8fea..8a62612 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = {
>>
>>   static const vshCmdOptDef opts_undefine[] = {
>>       {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")},
>> +    {"managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")},
> s/state/save/g, given my above thoughts.
>
>>       {NULL, 0, 0, NULL}
>>   };
>>
>> @@ -1419,6 +1420,16 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>       bool ret = true;
>>       const char *name = NULL;
>>       int id;
>> +    int flags = 0;
>> +    int managed_state = vshCommandOptBool(cmd, "managed-state");
>> +    int has_managed_state = 0;
>> +    int rc = -1;
>> +
>> +    if (managed_state)
>> +        flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE;
>> +
>> +    if (!managed_state)
>> +        flags = -1;
> I'm not sure if you need this line.  Instead...

This is for future use, we might introduce more flags. Such as
VIR_DOMAIN_UNDEFINE_SNAPSHOTS, VIR_DOMAIN_UNDEFINE_IMAGES.

>>
>>       if (!vshConnectionUsability(ctl, ctl->conn))
>>           return false;
>> @@ -1440,7 +1451,37 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>                                         VSH_BYNAME|VSH_BYUUID)))
>>           return false;
>>
>> -    if (virDomainUndefine(dom) == 0) {
>> +    has_managed_state = virDomainHasManagedSaveImage(dom, 0);
>> +    if (has_managed_state<  0)
>> +        return false;
>> +
>> +    if (flags == -1) {
> ...this conditional can just be if (!managed_state)

And this.

>> +        if (has_managed_state == 1) {
>> +            vshError(ctl,
>> +                     _("Refusing to undefine with managed state "
>> +                       "file exists"));
> Grammar:
>
> Refusing to undefine while managed save image exists
>
>> +            return false;
>> +        }
>> +
>> +        rc = virDomainUndefine(dom);
>> +    } else {
>> +        rc = virDomainUndefineFlags(dom, flags);
>> +
>> +        /* It might fail for virDomainUndefineFlags is not
>> +         * supported on older libvirt, try to undefine the
>> +         * domain with combo virDomainManagedSaveRemove and
>> +         * virDomainUndefine.
>> +         */
>> +        if (rc<  0) {
> Here, we should optimize by checking the error.  If the error is
> VIR_ERR_NO_SUPPORT, then we go with the fallback; but if it is anything
> else, then virDomainUndefineFlags exists but failed, and the fallback
> would also fail, so give up now.
>
>> +            if (has_managed_state&&
>> +                virDomainManagedSaveRemove(dom, 0)<  0)
>> +                return false;
> This early return...
>
>> +
>> +            rc = virDomainUndefine(dom);
>> +        }
>> +    }
>> +
>> +    if (rc == 0) {
>>           vshPrint(ctl, _("Domain %s has been undefined\n"), name);
>>       } else {
>>           vshError(ctl, _("Failed to undefine domain %s"), name);
> ...will lose out on this error message.
>
>>
>> -=item B<undefine>  I<domain-id>
>> +=item B<undefine>  I<domain-id>  I<--managed-state>
> managed-state (or managed-save, whatever we name it) is optional, so
> this should be:
>
> =item B<undefine>  I<domain-id>  [I<--managed-state>]
>
>>
>>   Undefine the configuration for an inactive domain. Since it's not running
>>   the domain name or UUID must be used as the I<domain-id>.
>>
>> +The I<--managed-save>  flag guarantees that any managed state (see the
>> +B<managesave>  command) is also cleaned up.  Without the flag, attempts
> s/managesave/managedsave/
>
>> +to undefine a domain with managed state will fail.
>> +
>>   =item B<vcpucount>  I<domain-id>   optional I<--maximum>  I<--current>
>>   I<--config>  I<--live>
>>
> Probably needs a v3.
>

Agree with the other comments.