Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

[Fam Zheng <famz@redhat.com>]

On Sat, 04/16 17:29, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 72 insertions(+), 17 deletions(-)
> >
> >diff --git a/qemu-img.c b/qemu-img.c
> >index 1697762..327be44 100644
> >--- a/qemu-img.c
> >+++ b/qemu-img.c
> pls fix help message near
> 
> static void QEMU_NORETURN help(void)
> {
>     const char *help_msg =
>            QEMU_IMG_VERSION
>            "usage: qemu-img command [command options]\n"
>            "QEMU disk image utility\n"
>            "\n"
>            "Command syntax:\n"
> #define DEF(option, callback, arg_string)        \
>            "  " arg_string "\n"
> #include "qemu-img-cmds.h"
> #undef DEF
> #undef GEN_DOCS
> 
> 
> IMHO img_create should also take lock if the image exists already
> to validate that there is no process on top of it.

Yes, good point.

> 
> 
> >@@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
> >      bool quiet = false;
> >      Error *local_err = NULL;
> >      bool image_opts = false;
> >+    bool nolock = false;
> >      fmt = NULL;
> >      output = NULL;
> >@@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
> >              {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> >              {0, 0, 0, 0}
> >          };
> >-        c = getopt_long(argc, argv, "hf:r:T:q",
> >+        c = getopt_long(argc, argv, "hf:r:T:qL",
> >                          long_options, &option_index);
> >          if (c == -1) {
> >              break;
> >@@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
> >          case 'q':
> >              quiet = true;
> >              break;
> >+        case 'L':
> >+            nolock = true;
> >+            break;
> I think that you could fix flags just here as done for 'r', i.e.
>                              flags |= BDRV_O_NO_LOCK
> 
> It would be better to switch all other places to this style. Some
> tweaks to old code would be necessary.
> 
> Though this is personal and does not block the review.

Yes, I can look into that.

Fam