qemu-img: refersh options and --help handling

[PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand

Posted by Michael Tokarev 8 months, 1 week ago

also remove bdrv_oflags handling (only list can use RO mode)
---
 qemu-img.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1e09b78d00..d9dfff2f07 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3541,7 +3541,7 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
     BlockDriverState *bs;
     QEMUSnapshotInfo sn;
     char *filename, *fmt = NULL, *snapshot_name = NULL;
-    int c, ret = 0, bdrv_oflags;
+    int c, ret = 0;
     int action = 0;
     bool quiet = false;
     Error *err = NULL;
@@ -3549,7 +3549,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
     bool force_share = false;
     int64_t rt;
 
-    bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
     for(;;) {
         static const struct option long_options[] = {
@@ -3583,7 +3582,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
                 return 0;
             }
             action = SNAPSHOT_LIST;
-            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
             break;
         case 'a':
             if (action) {
@@ -3629,9 +3627,14 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
     }
     filename = argv[optind++];
 
+    if (!action) {
+        action = SNAPSHOT_LIST;
+    }
+
     /* Open the image */
-    blk = img_open(image_opts, filename, fmt, bdrv_oflags, false, quiet,
-                   force_share);
+    blk = img_open(image_opts, filename, fmt,
+                   action == SNAPSHOT_LIST ? 0 : BDRV_O_RDWR,
+                   false, quiet, force_share);
     if (!blk) {
         return 1;
     }
-- 
2.39.2

Re: [PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand

Posted by Daniel P. Berrangé 7 months, 3 weeks ago

On Sat, Feb 10, 2024 at 12:22:33AM +0300, Michael Tokarev wrote:
> also remove bdrv_oflags handling (only list can use RO mode)
> ---
>  qemu-img.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

I'd suggest docs/tools/qemu-img.rst should also be updated to say

 Lists all snapshots in the given image (default action)

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 1e09b78d00..d9dfff2f07 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3541,7 +3541,7 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn;
>      char *filename, *fmt = NULL, *snapshot_name = NULL;
> -    int c, ret = 0, bdrv_oflags;
> +    int c, ret = 0;
>      int action = 0;
>      bool quiet = false;
>      Error *err = NULL;
> @@ -3549,7 +3549,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
>      bool force_share = false;
>      int64_t rt;
>  
> -    bdrv_oflags = BDRV_O_RDWR;
>      /* Parse commandline parameters */
>      for(;;) {
>          static const struct option long_options[] = {
> @@ -3583,7 +3582,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
>                  return 0;
>              }
>              action = SNAPSHOT_LIST;
> -            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
>              break;
>          case 'a':
>              if (action) {
> @@ -3629,9 +3627,14 @@ static int img_snapshot(const img_cmd_t *ccmd, int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    if (!action) {
> +        action = SNAPSHOT_LIST;
> +    }
> +
>      /* Open the image */
> -    blk = img_open(image_opts, filename, fmt, bdrv_oflags, false, quiet,
> -                   force_share);
> +    blk = img_open(image_opts, filename, fmt,
> +                   action == SNAPSHOT_LIST ? 0 : BDRV_O_RDWR,
> +                   false, quiet, force_share);
>      if (!blk) {
>          return 1;
>      }
> -- 
> 2.39.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand

Posted by Michael Tokarev 7 months, 3 weeks ago

20.02.2024 20:45, Daniel P. Berrangé wrote:
> On Sat, Feb 10, 2024 at 12:22:33AM +0300, Michael Tokarev wrote:
>> also remove bdrv_oflags handling (only list can use RO mode)
>> ---
>>   qemu-img.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> I'd suggest docs/tools/qemu-img.rst should also be updated to say

qemu-img.rst needs to be updated, significantly more than this.
I especially avoided updating this one for now because I'd love
to agree with the options first, more or less, or else it'd be
large double-work.  Also, there's an open question still, if
we prefer to keep docs and code in sync manually or to use some
automation like qemu-img-cmdx.hx now.  I for one don't see how
this can be done in a reasonable way.

I plan to update it past this series.

Thanks!

/mjt

Re: [PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand

Posted by Michael Tokarev 7 months, 3 weeks ago

20.02.2024 21:51, Michael Tokarev wrote:
> 20.02.2024 20:45, Daniel P. Berrangé wrote:
>> On Sat, Feb 10, 2024 at 12:22:33AM +0300, Michael Tokarev wrote:
>>> also remove bdrv_oflags handling (only list can use RO mode)
>>> ---
>>>   qemu-img.c | 13 ++++++++-----
>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> I'd suggest docs/tools/qemu-img.rst should also be updated to say
> 
> qemu-img.rst needs to be updated, significantly more than this.

Actually, - yes, you're right in this case.  Added.  A good suggestion.

Thanks,

/mjt