From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asVMd-0002pi-La for qemu-devel@nongnu.org; Tue, 19 Apr 2016 09:06:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asVMZ-00078w-JL for qemu-devel@nongnu.org; Tue, 19 Apr 2016 09:06:47 -0400 Date: Tue, 19 Apr 2016 20:59:31 +0800 From: Fam Zheng Message-ID: <20160419125931.GF28572@ad-mail.usersys.redhat.com> References: <1460690887-32751-1-git-send-email-famz@redhat.com> <1460690887-32751-10-git-send-email-famz@redhat.com> <57124C2F.1070607@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57124C2F.1070607@openvz.org> Subject: Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Kevin Wolf , Max Reitz , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@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 > >--- > > 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