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

[Peter Lieven <pl@kamp.de>]

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 <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.

division by zero is x / 0 not 0 / x.

0 / x = 0
x / 0 = undef

Peter