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

[Leandro Dorileo]

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 <address@hidden>
> >---
> >  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.


> 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
>   address@hidden | http://www.kamp.de
> 
>   Geschäftsführer: Heiner Lante | Michael Lante
>   Amtsgericht Duisburg | HRB Nr. 12154
>   USt-Id-Nr.: DE 120607556
> 
> ...........................................................
> 
> 

-- 
Leandro Dorileo