diff mbox

[for-2.7,v2,09/17] qemu-img: Add "-L" option to sub commands

Message ID 1460690887-32751-10-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 15, 2016, 3:27 a.m. UTC 
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(-)

Comments

Denis V. Lunev April 16, 2016, 2:29 p.m. UTC | #1 
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.


> @@ -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.
Denis V. Lunev April 16, 2016, 2:30 p.m. UTC | #2 
On 04/16/2016 05:29 PM, 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
>
ah, I see this in the next patch. Though the question about img_create
is still actual.



>
> IMHO img_create should also take lock if the image exists already
> to validate that there is no process on top of it.
>
>
>> @@ -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.
Fam Zheng April 19, 2016, 12:59 p.m. UTC | #3 
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
diff mbox

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 1697762..327be44 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -684,6 +688,7 @@  static int img_check(int argc, char **argv)
         return 1;
     }
 
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -804,6 +809,7 @@  static int img_commit(int argc, char **argv)
     Error *local_err = NULL;
     CommonBlockJobCBInfo cbi;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
@@ -815,7 +821,7 @@  static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, "f:ht:b:dpqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -845,6 +851,9 @@  static int img_commit(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -877,6 +886,7 @@  static int img_commit(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR | BDRV_O_UNMAP;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1135,6 +1145,7 @@  static int img_compare(int argc, char **argv)
     uint64_t progress_base;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1144,7 +1155,7 @@  static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, "hf:F:T:pqsL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1172,6 +1183,9 @@  static int img_compare(int argc, char **argv)
         case 's':
             strict = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -1211,6 +1225,7 @@  static int img_compare(int argc, char **argv)
     qemu_progress_init(progress, 2.0);
 
     flags = 0;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -1742,6 +1757,7 @@  static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1757,7 +1773,7 @@  static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
+        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1846,6 +1862,9 @@  static int img_convert(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'n':
             skip_create = 1;
             break;
@@ -1896,6 +1915,7 @@  static int img_convert(int argc, char **argv)
     }
 
     src_flags = 0;
+    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
@@ -2045,6 +2065,7 @@  static int img_convert(int argc, char **argv)
     }
 
     flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -2225,12 +2246,14 @@  static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 static ImageInfoList *collect_image_info_list(bool image_opts,
                                               const char *filename,
                                               const char *fmt,
-                                              bool chain)
+                                              bool chain,
+                                              bool nolock)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
     Error *err = NULL;
+    int flags;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -2247,8 +2270,9 @@  static ImageInfoList *collect_image_info_list(bool image_opts,
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
-        blk = img_open(image_opts, filename, fmt,
-                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false);
+        flags = BDRV_O_NO_BACKING | BDRV_O_NO_IO;
+        flags |= nolock ? BDRV_O_NO_LOCK : 0;
+        blk = img_open(image_opts, filename, fmt, flags, false, false);
         if (!blk) {
             goto err;
         }
@@ -2301,6 +2325,7 @@  static int img_info(int argc, char **argv)
     ImageInfoList *list;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -2315,7 +2340,7 @@  static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2328,6 +2353,9 @@  static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2368,7 +2396,7 @@  static int img_info(int argc, char **argv)
         return 1;
     }
 
-    list = collect_image_info_list(image_opts, filename, fmt, chain);
+    list = collect_image_info_list(image_opts, filename, fmt, chain, nolock);
     if (!list) {
         return 1;
     }
@@ -2515,6 +2543,8 @@  static int img_map(int argc, char **argv)
     int ret = 0;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
+    int flags;
 
     fmt = NULL;
     output = NULL;
@@ -2528,7 +2558,7 @@  static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2541,6 +2571,9 @@  static int img_map(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2578,7 +2611,8 @@  static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false);
+    flags = nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, false);
     if (!blk) {
         return 1;
     }
@@ -2641,6 +2675,7 @@  static int img_snapshot(int argc, char **argv)
     bool quiet = false;
     Error *err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2651,7 +2686,7 @@  static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, "la:c:d:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2696,6 +2731,9 @@  static int img_snapshot(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2722,6 +2760,7 @@  static int img_snapshot(int argc, char **argv)
         return 1;
     }
 
+    bdrv_oflags |= nolock ? BDRV_O_NO_LOCK : 0;
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet);
     if (!blk) {
@@ -2791,6 +2830,7 @@  static int img_rebase(int argc, char **argv)
     bool quiet = false;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -2805,7 +2845,7 @@  static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, "hf:F:b:upt:T:qL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2836,6 +2876,9 @@  static int img_rebase(int argc, char **argv)
         case 'T':
             src_cache = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -2876,6 +2919,7 @@  static int img_rebase(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -3134,6 +3178,8 @@  static int img_resize(int argc, char **argv)
     BlockBackend *blk = NULL;
     QemuOpts *param;
     Error *local_err = NULL;
+    int flags;
+    bool nolock = false;
 
     static QemuOptsList resize_options = {
         .name = "resize_options",
@@ -3168,7 +3214,7 @@  static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, "f:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3184,6 +3230,9 @@  static int img_resize(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3236,8 +3285,9 @@  static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = img_open(image_opts, filename, fmt,
-                   BDRV_O_RDWR, false, quiet);
+    flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, quiet);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3299,6 +3349,7 @@  static int img_amend(int argc, char **argv)
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -3308,7 +3359,7 @@  static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, "ho:f:t:pqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3345,6 +3396,9 @@  static int img_amend(int argc, char **argv)
             case 'q':
                 quiet = true;
                 break;
+            case 'L':
+                nolock = true;
+                break;
             case OPTION_OBJECT:
                 opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                                optarg, true);
@@ -3391,6 +3445,7 @@  static int img_amend(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);