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

Haruka Ohata posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20221129070033.162153-1-ohata.haruka@fujitsu.com
There is a newer version of this series
tests/virsh-snapshot   | 3 +++
tools/virsh-snapshot.c | 6 +++++-
2 files changed, 8 insertions(+), 1 deletion(-)
[PATCH v3] virsh: Add message to terminal when running snapshot-revert
Posted by Haruka Ohata 1 year, 5 months ago
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@fujitsu.com>
---
 tests/virsh-snapshot   | 3 +++
 tools/virsh-snapshot.c | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

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..b7ec675e61 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1783,7 +1783,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
         result = virDomainRevertToSnapshot(snapshot, flags);
     }
 
-    return result >= 0;
+    if (result < 0)
+        vshError(ctl, _("Failed to revert snapshot %s"), name);
+    else
+        vshPrintExtra(ctl, _("Domain snapshot %s reverted\n"), name);
+    return result;
 }
 
 /*
-- 
2.38.1
Re: [PATCH v3] virsh: Add message to terminal when running snapshot-revert
Posted by Peter Krempa 1 year, 5 months ago
On Tue, Nov 29, 2022 at 16:00:33 +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@fujitsu.com>
> ---
>  tests/virsh-snapshot   | 3 +++
>  tools/virsh-snapshot.c | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> 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..b7ec675e61 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -1783,7 +1783,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
>          result = virDomainRevertToSnapshot(snapshot, flags);
>      }
>  
> -    return result >= 0;

This function's return value is a boolean. Expected retruns are 'true'
in case when the command was successful and false if not. Here it's
achieved by checking that the return code from virDomainRevertToSnapshot
is zero or positive: 

  result   return value

  -1       false
  0        true
  1        true

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

Now you changed it to directly use 'result'. This means that a typecast
will be performed. Casting an int to a boolean has the following truth
table

result    return value
 -1       true
 0        false
 1        true

Thus you can see it's wrong. Effectively, since
virDomainRevertToSnapshot returns only -1 or 0, you've inverted the
return values of this command.

You don't need to change the 'return' statement at all.