On Tue, Nov 20, 2012 at 1:14 AM, Stefan Hajnoczi <stefa...@gmail.com> 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.
