10:05 p.m.
于 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.