From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1arRFS-0004Q1-8O for qemu-devel@nongnu.org; Sat, 16 Apr 2016 10:30:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1arRFR-00070U-Dl for qemu-devel@nongnu.org; Sat, 16 Apr 2016 10:30:58 -0400 References: <1460690887-32751-1-git-send-email-famz@redhat.com> <1460690887-32751-10-git-send-email-famz@redhat.com> <57124C2F.1070607@openvz.org> From: "Denis V. Lunev" Message-ID: <57124C94.6080700@virtuozzo.com> Date: Sat, 16 Apr 2016 17:30:44 +0300 MIME-Version: 1.0 In-Reply-To: <57124C2F.1070607@openvz.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit 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: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Max Reitz , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com 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 >> --- >> 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.