Re: [Qemu-devel] [PATCH V5 09/10] Use QemuOpts support in block layer

[Dong Xu Wang]

On Tue, Nov 20, 2012 at 1:14 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Thu, Nov 01, 2012 at 05:12:29PM +0800, Dong Xu Wang wrote:
>> diff --git a/block/qcow.c b/block/qcow.c
>> index b239c82..9bb736a 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs)
>>      error_free(s->migration_blocker);
>>  }
>>
>> -static int qcow_create(const char *filename, QEMUOptionParameter *options)
>> +static int qcow_create(const char *filename, QemuOpts *opts)
>>  {
>>      int header_size, backing_filename_len, l1_size, shift, i;
>>      QCowHeader header;
>> @@ -662,19 +662,14 @@ static int qcow_create(const char *filename, 
>> QEMUOptionParameter *options)
>>      int ret;
>>      BlockDriverState *qcow_bs;
>>
>> -    /* Read out options */
>> -    while (options && options->name) {
>> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> -            total_size = options->value.n / 512;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>> -            backing_file = options->value.s;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
>> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
>> -        }
>> -        options++;
>> +    /* Read out opts */
>> +    total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / 512;
>> +    backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>> +    if (qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, 0)) {
>> +        flags |= BLOCK_FLAG_ENCRYPT;
>>      }
>>
>> -    ret = bdrv_create_file(filename, options);
>> +    ret = bdrv_create_file(filename, opts);
>
> Previously options would be an empty QEMUOptionParameter (because the
> while loop iterated until options->name).  Now you are passing the same
> opts for creating this image file, this seems wrong.  Should this be
> bdrv_create_file(filename, NULL)?
>
Yep.

>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c1ff31f..9cea051 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1180,7 +1180,7 @@ static int preallocate(BlockDriverState *bs)
>>  static int qcow2_create2(const char *filename, int64_t total_size,
>>                           const char *backing_file, const char 
>> *backing_format,
>>                           int flags, size_t cluster_size, int prealloc,
>> -                         QEMUOptionParameter *options, int version)
>> +                         QemuOpts *opts, int version)
>
> The opts argument can be dropped since it is not used.
>
Okay.

>> @@ -1314,7 +1314,7 @@ out:
>>      return ret;
>>  }
>>
>> -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
>> +static int qcow2_create(const char *filename, QemuOpts *opts)
>>  {
>>      const char *backing_file = NULL;
>>      const char *backing_fmt = NULL;
>> @@ -1323,45 +1323,41 @@ static int qcow2_create(const char *filename, 
>> QEMUOptionParameter *options)
>>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
>>      int prealloc = 0;
>>      int version = 2;
>> +    const char *buf;
>>
>>      /* Read out options */
>> -    while (options && options->name) {
>> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> -            sectors = options->value.n / 512;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>> -            backing_file = options->value.s;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
>> -            backing_fmt = options->value.s;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
>> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
>> -        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
>> -            if (options->value.n) {
>> -                cluster_size = options->value.n;
>> -            }
>> -        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
>> -            if (!options->value.s || !strcmp(options->value.s, "off")) {
>> -                prealloc = 0;
>> -            } else if (!strcmp(options->value.s, "metadata")) {
>> -                prealloc = 1;
>> -            } else {
>> -                fprintf(stderr, "Invalid preallocation mode: '%s'\n",
>> -                    options->value.s);
>> -                return -EINVAL;
>> -            }
>> -        } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
>> -            if (!options->value.s || !strcmp(options->value.s, "0.10")) {
>> -                version = 2;
>> -            } else if (!strcmp(options->value.s, "1.1")) {
>> -                version = 3;
>> -            } else {
>> -                fprintf(stderr, "Invalid compatibility level: '%s'\n",
>> -                    options->value.s);
>> -                return -EINVAL;
>> -            }
>> -        } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
>> -            flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
>> -        }
>> -        options++;
>
> The old code worked when options == NULL.  I think the new code should
> also work when opts == NULL:
>
> if (opts) {
>     ....
> }
Okay.

>
> This should be done for all block drivers or the way that
> bdrv_create(..., NULL) works should be changed so that block drivers
> don't have to check for opts == NULL.
>
>> @@ -674,19 +687,14 @@ static int64_t 
>> raw_get_allocated_file_size(BlockDriverState *bs)
>>      return (int64_t)st.st_blocks * 512;
>>  }
>>
>> -static int raw_create(const char *filename, QEMUOptionParameter *options)
>> +static int raw_create(const char *filename, QemuOpts *opts)
>>  {
>>      int fd;
>>      int result = 0;
>>      int64_t total_size = 0;
>>
>> -    /* Read out options */
>> -    while (options && options->name) {
>> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
>> -        }
>> -        options++;
>> -    }
>> +    total_size = opts ?
>> +        qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0;
>
> total_size is initialized to 0 above, so this line can be made more
> readable like this:
>
> if (opts) {
>     total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0);
> }
Okay.

>
>> @@ -1081,20 +1079,15 @@ static int fd_open(BlockDriverState *bs)
>>
>>  #endif /* !linux && !FreeBSD */
>>
>> -static int hdev_create(const char *filename, QEMUOptionParameter *options)
>> +static int hdev_create(const char *filename, QemuOpts *opts)
>>  {
>>      int fd;
>>      int ret = 0;
>>      struct stat stat_buf;
>>      int64_t total_size = 0;
>>
>> -    /* Read out options */
>> -    while (options && options->name) {
>> -        if (!strcmp(options->name, "size")) {
>> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
>> -        }
>> -        options++;
>> -    }
>> +    total_size = opts ?
>> +        qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0;
>
> Same here.
>

Okay. Thank you Stefan, will correct them in next version.