From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37547) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGZNN-0005op-4P for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:33:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGZNH-0000FF-4y for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:33:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37045) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGZNG-0000F9-T3 for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:33:35 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1KJXXFV012466 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 20 Feb 2014 14:33:34 -0500 Date: Thu, 20 Feb 2014 14:33:30 -0500 From: Jeff Cody Message-ID: <20140220193330.GE30664@localhost.localdomain> References: <1392908243-8835-1-git-send-email-kwolf@redhat.com> <1392908243-8835-4-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1392908243-8835-4-git-send-email-kwolf@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: mreitz@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote: > Instead of ignoring all option values but the last one, multiple -o > options now have the same meaning as having a single option with all > settings in the order of their respective -o options. > > Signed-off-by: Kevin Wolf > --- > qemu-img.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 770e4e6..ba6e82d 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1160,6 +1160,9 @@ static int img_convert(int argc, char **argv) > Error *local_err = NULL; > QemuOpts *sn_opts = NULL; > > + /* Initialize before goto out */ > + qemu_progress_init(progress, 1.0); > + > fmt = NULL; > out_fmt = "raw"; > cache = "unsafe"; > @@ -1191,13 +1194,21 @@ static int img_convert(int argc, char **argv) > case 'e': > error_report("option -e is deprecated, please use \'-o " > "encryption\' instead!"); > - return 1; > + ret = -1; > + goto out; > case '6': > error_report("option -6 is deprecated, please use \'-o " > "compat6\' instead!"); > - return 1; > + ret = -1; > + goto out; > case 'o': > - options = optarg; > + if (!options) { > + options = g_strdup(optarg); > + } else { > + char *old_options = options; > + options = g_strdup_printf("%s,%s", options, optarg); > + g_free(old_options); > + } > break; > case 's': > snapshot_name = optarg; > @@ -1208,7 +1219,8 @@ static int img_convert(int argc, char **argv) > if (!sn_opts) { > error_report("Failed in parsing snapshot param '%s'", > optarg); > - return 1; > + ret = -1; > + goto out; > } > } else { > snapshot_name = optarg; > @@ -1221,7 +1233,8 @@ static int img_convert(int argc, char **argv) > sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B); > if (sval < 0 || *end) { > error_report("Invalid minimum zero buffer size for sparse output specified"); > - return 1; > + ret = -1; > + goto out; > } There is a 'return -1' that is missed, that I think needs to be 'goto out;' for cleanup. It is right after the call to bdrv_parse_cache_flags() in the img_convert() function: ret = bdrv_parse_cache_flags(cache, &flags); if (ret < 0) { error_report("Invalid cache option: %s", cache); return -1; } Looks like this has been this way for a while, though. And this is an instance (the only instance) where img_convert returns -1 instead of 0 or 1. I'm not sure the implications if that changed, and became '1', so we'd probably want to preserve the return value. Since this doesn't have anything to do with this patch (although leaking 'options' now, in addition to the other stuff leaked), I'll go ahead and give my reviewed-by, and we can fix this in another follow-up patch: Reviewed-by: Jeff Cody > > min_sparse = sval / BDRV_SECTOR_SIZE; > @@ -1253,10 +1266,7 @@ static int img_convert(int argc, char **argv) > > out_filename = argv[argc - 1]; > > - /* Initialize before goto out */ > - qemu_progress_init(progress, 1.0); > - > - if (options && is_help_option(options)) { > + if (options && has_help_option(options)) { > ret = print_block_option_help(out_filename, out_fmt); > goto out; > } > @@ -1649,6 +1659,7 @@ out: > free_option_parameters(create_options); > free_option_parameters(param); > qemu_vfree(buf); > + g_free(options); > if (sn_opts) { > qemu_opts_del(sn_opts); > } > -- > 1.8.1.4 > >