[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [[PATCHv2 3/2]] virsh: add --graceful switch to destroy command



On 03.02.2012 20:34, Laine Stump wrote:
> This allows virsh to use the new VIR_DOMAIN_DESTROY_GRACEUL flag for
> virDomainDestroyFlags.
> ---
> Okay, how about this one :-)
> 
>  tools/virsh.c   |    9 ++++++++-
>  tools/virsh.pod |    6 +++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 42985a9..6a151e6 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -4264,6 +4264,7 @@ static const vshCmdInfo info_destroy[] = {
>  
>  static const vshCmdOptDef opts_destroy[] = {
>      {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> +    {"graceful", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("needs rawio capability")},
>      {NULL, 0, 0, NULL}
>  };
>  
> @@ -4273,6 +4274,7 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd)
>      virDomainPtr dom;
>      bool ret = true;
>      const char *name;
> +    int result;

I tend to define unsigned flags = 0; here, even when they'll be used
only for one flag right now, but it will simplify patches adding a new one.
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return false;
> @@ -4280,7 +4282,12 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, &name)))
>          return false;
>  
> -    if (virDomainDestroy(dom) == 0) {
> +    if (vshCommandOptBool(cmd, "graceful"))
> +       result = virDomainDestroyFlags(dom, VIR_DOMAIN_DESTROY_GRACEFUL);
> +    else
> +       result = virDomainDestroy(dom);
> +
> +    if (result == 0) {

So this chunk will look like:

if (vshCommandOptBool(cmd, "graceful"))
  flags |= VIR_DOMAIN_DESTROY_GRACEFUL;

if (flags)
  result = virDomainDestroyFlags(dom, flags);
else
  result = virDomainDestroy(dom);

if (result == 0) {

But this is not a show stopper for me.
>          vshPrint(ctl, _("Domain %s destroyed\n"), name);
>      } else {
>          vshError(ctl, _("Failed to destroy domain %s"), name);
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 10b51ff..a85da13 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -458,7 +458,7 @@ Flag I<--title> selects operation on the title field instead of description.
>  If neither of I<--edit> and I<--new_desc> are specified the note or description
>  is displayed instead of being modified.
>  
> -=item B<destroy> I<domain-id>
> +=item B<destroy> I<domain-id> [I<--graceful>]
>  
>  Immediately terminate the domain domain-id.  This doesn't give the domain
>  OS any chance to react, and it's the equivalent of ripping the power
> @@ -472,6 +472,10 @@ be lost once the guest stops running, but the snapshot contents still
>  exist, and a new domain with the same name and UUID can restore the
>  snapshot metadata with B<snapshot-create>.
>  
> +If I<--graceful> is specified, don't resort to extreme measures
> +(e.g. SIGKILL) when the guest doesn't stop after a reasonable timeout;
> +return an error instead.
> +
>  =item B<domblkstat> I<domain> I<block-device> [I<--human>]
>  
>  Get device block stats for a running domain.  A I<block-device> corresponds

Nice, you haven't forgot the documentation.

Overall, ACK, regardless you'll change that nit or not.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]