[libvirt] [PATCH v2 7/8] undefine: Extend virsh undefine to support the new API
Osier Yang
jyang at redhat.com
Sat Jul 16 03:05:49 UTC 2011
于 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.
More information about the libvir-list
mailing list