From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9gNM-0007NP-7A for qemu-devel@nongnu.org; Tue, 22 Jul 2014 16:09:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X9gNB-0006xN-1w for qemu-devel@nongnu.org; Tue, 22 Jul 2014 16:09:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13906) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9gNA-0006xI-PG for qemu-devel@nongnu.org; Tue, 22 Jul 2014 16:09:16 -0400 Message-ID: <53CEC4FF.9030705@redhat.com> Date: Tue, 22 Jul 2014 22:09:35 +0200 From: Max Reitz MIME-Version: 1.0 References: <1405802159-2355-1-git-send-email-mreitz@redhat.com> <1405802159-2355-3-git-send-email-mreitz@redhat.com> <53CD386F.307@redhat.com> In-Reply-To: <53CD386F.307@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi On 21.07.2014 17:57, Eric Blake wrote: > On 07/19/2014 02:35 PM, Max Reitz wrote: >> qemu-img amend may extensively modify the target image, depending on the >> options to be amended (e.g. conversion to qcow2 compat level 0.10 from >> 1.1 for an image with many unallocated zero clusters). Therefore it >> makes sense to allow the user to specify the cache mode to be used. > Extensive modifications implies long-running operation - the 'amend' > subcommand is a good candidate for the -p progress meter option. But > that would be a separate patch. Yes, this is on my to do list anyway. >> Signed-off-by: Max Reitz >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 19 +++++++++++++++---- >> qemu-img.texi | 2 +- >> 3 files changed, 18 insertions(+), 7 deletions(-) >> >> >> + cache = BDRV_DEFAULT_CACHE; >> for (;;) { >> - c = getopt(argc, argv, "hqf:o:"); >> + c = getopt(argc, argv, "hqf:t:o:"); >> if (c == -1) { >> break; >> } >> @@ -2805,6 +2807,9 @@ static int img_amend(int argc, char **argv) >> case 'f': >> fmt = optarg; >> break; >> + case 't': >> + cache = optarg; >> + break; >> case 'q': > > Pre-existing, so I won't hold up review, but I'm a big fan of having the > switch block in the same order as the getopt string (that is, we listed > 'q' before 'f' in the string above, so the cases are out-of-order with > respect to that string). The fix can go either way (reshuffle the case > statements, or reorder the optstring above). [and for the truly OCD, I > prefer the optstring in case-insensitive alphabetical order "f:ho:qt:", > because then it's easier to scan the string to see what letters are > still available for new options - but that's asking a bit much] Right, in fact I thought of you when I touched this block; since it was already completely out of order, I decided for the minimal change, however. I'll redo it in v2. I personally somehow like to have common switches like -q at the end, I don't know why. For now, I'll just reshuffle the getopt() string. > Reviewed-by: Eric Blake Thanks, Max