[PATCH v2] virsh: Add message to terminal when running snapshot-revert

[Peter Krempa]

On Mon, Nov 28, 2022 at 16:33:40 +0900, Haruka Ohata wrote:
> When running virsh snapshot-* command, such as snapshot-create-as /
> snapshot-delete, it prints a result message.
> On the other hand virsh snapshot-revert command doesn't print a result
> message.
> 
> So, This patch fixes to add message when running virsh snapshot-revert
> command.
> ---
>     # virsh snapshot-create-as vm1 test1
>     Domain snapshot test01 created
>     # virsh snapshot-revert vm1 test1
> 
>     # virsh snapshot-delete vm1 test1
>     Domain snapshot test01 deleted
> 
>     #
> ---
> 
> Signed-off-by: Haruka Ohata <ohata.haruka at fujitsu.com>
> ---
>  tests/virsh-snapshot   |  3 +++
>  tools/virsh-snapshot.c | 10 +++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
> index 4c64bb537b..b09273917b 100755
> --- a/tests/virsh-snapshot
> +++ b/tests/virsh-snapshot
> @@ -100,11 +100,14 @@ Domain snapshot s1 created
>  
>  Domain snapshot s3 created
>  Domain snapshot s2 created
> +Domain snapshot s3 reverted
>  
>  Domain snapshot s6 created
>  Domain snapshot s5 created
> +Domain snapshot s6 reverted
>  
>  Domain snapshot s4 created
> +Domain snapshot s1 reverted
>  
>  Domain snapshot s7 created
>  Domain snapshot s8 created
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 8fa64ba903..4576bf1020 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -1745,7 +1745,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
>  };
>  
>  static bool
> -cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
> +cmdSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
>  {
>      g_autoptr(virshDomain) dom = NULL;
>      const char *name = NULL;
> @@ -1783,7 +1783,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
>          result = virDomainRevertToSnapshot(snapshot, flags);
>      }

The full context above:


    result = virDomainRevertToSnapshot(snapshot, flags);
    if (result < 0 && force &&
        last_error->code == VIR_ERR_SNAPSHOT_REVERT_RISKY) {
        flags |= VIR_DOMAIN_SNAPSHOT_REVERT_FORCE;
        vshResetLibvirtError();
        result = virDomainRevertToSnapshot(snapshot, flags);
    }

    return result >= 0;

>  
> -    return result >= 0;
> +    if (virDomainRevertToSnapshot(snapshot, flags) < 0)
> +        vshError(ctl, _("Failed to revert snapshot %s"), name);
> +    else
> +        vshPrintExtra(ctl, _("Domain snapshot %s reverted\n"), name);
> +    return result;

Here you call virDomainRevertToSnapshot again. That makes no sense as
the call to revert the snapshot was already done. Additionally the logic
above is required to handle the --force flag properly.

You must make the decision based on the 'result' variable, not an
additional call to virDomainRevertToSnapshot.

>  }
>  
>  /*
> @@ -1916,7 +1920,7 @@ const vshCmdDef snapshotCmds[] = {
>       .flags = 0
>      },
>      {.name = "snapshot-revert",
> -     .handler = cmdDomainSnapshotRevert,
> +     .handler = cmdSnapshotRevert,

Please separate the function name change to a separate commit as it
doesn't relate to what this commit is doing.

>       .opts = opts_snapshot_revert,
>       .info = info_snapshot_revert,
>       .flags = 0
> -- 
> 2.38.1
>