Re: [Qemu-devel] [PATCH 09/26] iscsi: migrate iscsi driver QemuOptionParameter usage

[Leandro Dorileo <l@dorileo.org>]

On Fri, Mar 21, 2014 at 01:31:42PM +0000, 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 <l@dorileo.org>
> > >---
> > >  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.

Ok, qemu-img does guarantee the img_size value...

> 
> 
> > Peter
> > >      bs->opaque = g_malloc0(sizeof(struct IscsiLun));
> > >@@ -1451,13 +1447,17 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> > >      return 0;
> > >  }
> > >-static QEMUOptionParameter iscsi_create_options[] = {
> > >-    {
> > >-        .name = BLOCK_OPT_SIZE,
> > >-        .type = OPT_SIZE,
> > >-        .help = "Virtual disk size"
> > >+static QemuOptsList iscsi_create_options = {
> > >+    .name = "iscsi_create_options",
> > >+    .head = QTAILQ_HEAD_INITIALIZER(iscsi_create_options.head),
> > >+    .desc = {
> > >+        {
> > >+            .name = BLOCK_OPT_SIZE,
> > >+            .type = QEMU_OPT_SIZE,
> > >+            .help = "Virtual disk size"
> > >+        },
> > >+        { NULL }
> > >      },
> > >-    { NULL }
> > >  };
> > >  static BlockDriver bdrv_iscsi = {
> > >@@ -1469,7 +1469,7 @@ static BlockDriver bdrv_iscsi = {
> > >      .bdrv_file_open  = iscsi_open,
> > >      .bdrv_close      = iscsi_close,
> > >      .bdrv_create     = iscsi_create,
> > >-    .create_options  = iscsi_create_options,
> > >+    .create_options  = &iscsi_create_options,
> > >      .bdrv_reopen_prepare  = iscsi_reopen_prepare,
> > >      .bdrv_getlength  = iscsi_getlength,
> > 
> > 
> > -- 
> > 
> > Mit freundlichen Grüßen
> > 
> > Peter Lieven
> > 
> > ...........................................................
> > 
> >   KAMP Netzwerkdienste GmbH
> >   Vestische Str. 89-91 | 46117 Oberhausen
> >   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
> >   pl@kamp.de | http://www.kamp.de
> > 
> >   Geschäftsführer: Heiner Lante | Michael Lante
> >   Amtsgericht Duisburg | HRB Nr. 12154
> >   USt-Id-Nr.: DE 120607556
> > 
> > ...........................................................
> > 
> > 
> 
> -- 
> Leandro Dorileo

-- 
Leandro Dorileo