From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47285) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQzve-0007I1-NP for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:56:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQzvZ-0000oK-Qx for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:56:10 -0400 Received: from mail-ob0-f170.google.com ([209.85.214.170]:59991) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQzvZ-0000oE-Mu for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:56:05 -0400 Received: by mail-ob0-f170.google.com with SMTP id uz6so2515748obc.29 for ; Fri, 21 Mar 2014 06:56:05 -0700 (PDT) Date: Fri, 21 Mar 2014 13:54:50 +0000 From: Leandro Dorileo Message-ID: <20140321135450.GC22259@dorilex> References: <1395360813-2833-1-git-send-email-l@dorileo.org> <1395360813-2833-10-git-send-email-l@dorileo.org> <532BDFA0.7050807@kamp.de> <20140321133142.GA22259@dorilex> <532C41CB.5080306@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <532C41CB.5080306@kamp.de> Subject: Re: [Qemu-devel] [PATCH 09/26] iscsi: migrate iscsi driver QemuOptionParameter usage List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: Kevin Wolf , Fam Zheng , Stefan Hajnoczi , Stefan Weil , Jeff Cody , "Richard W.M. Jones" , qemu-devel@nongnu.org, Max Reitz , MORITA Kazutaka , Ronnie Sahlberg , Josh Durgin , Anthony Liguori , Paolo Bonzini , Liu Yuan , Luiz Capitulino , Markus Armbruster , Benoit Canet On Fri, Mar 21, 2014 at 02:42:35PM +0100, Peter Lieven wrote: > On 21.03.2014 14:31, Leandro Dorileo wrote: > >On Fri, Mar 21, 2014 at 07:43:44AM +0100, Peter Lieven wrote: > >>On 21.03.2014 01:13, Leandro Dorileo wrote: > >>>Do the directly migration from QemuOptionParameter to QemuOpts on > >>>iscsi block driver. > >>> > >>>Signed-off-by: Leandro Dorileo > >>>--- > >>> block/iscsi.c | 32 ++++++++++++++++---------------- > >>> 1 file changed, 16 insertions(+), 16 deletions(-) > >>> > >>>diff --git a/block/iscsi.c b/block/iscsi.c > >>>index b490e98..85252e7 100644 > >>>--- a/block/iscsi.c > >>>+++ b/block/iscsi.c > >>>@@ -1125,7 +1125,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, > >>> QemuOpts *opts; > >>> Error *local_err = NULL; > >>> const char *filename; > >>>- int i, ret; > >>>+ int i, ret = 0; > >>why? is there a chance that ret remains uninitialized? > >Yep, my compiler tells me so: > > > >block/iscsi.c:1128:12: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > > >>> if ((BDRV_SECTOR_SIZE % 512) != 0) { > >>> error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. " > >>>@@ -1382,8 +1382,7 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset) > >>> return 0; > >>> } > >>>-static int iscsi_create(const char *filename, QEMUOptionParameter *options, > >>>- Error **errp) > >>>+static int iscsi_create(const char *filename, QemuOpts *options, Error **errp) > >>> { > >>> int ret = 0; > >>> int64_t total_size = 0; > >>>@@ -1393,12 +1392,9 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options, > >>> bs = bdrv_new(""); > >>>- /* Read out options */ > >>>- while (options && options->name) { > >>>- if (!strcmp(options->name, "size")) { > >>>- total_size = options->value.n / BDRV_SECTOR_SIZE; > >>>- } > >>>- options++; > >>>+ total_size = qemu_opt_get_size(options, BLOCK_OPT_SIZE, 0); > >>>+ if (total_size) { > >>>+ total_size = total_size / BDRV_SECTOR_SIZE; > >>> } > >>you don't need the if condition. 0 / BDRV_SECTOR_SIZE = 0. > >> > >I'm not sure, bdrv_img_create() will set BLOCK_OPT_SIZE with img_size, we have no guarantee on the > >value passed to bdrv_img_create(), we don't check img_size value there, having said that can't > >we run on division by zero here? The previous code wasn't checking it but I wonder if the problem > >wasn't there already. > > division by zero is x / 0 not 0 / x. > > 0 / x = 0 > x / 0 = undef > Yep, true. -- Leandro Dorileo