[Qemu-devel] Re: [PATCH 1/3] qemu-img.c: Re-factor img_create()

[Jes Sorensen]

On 12/16/10 12:35, Kevin Wolf wrote:
> Am 16.12.2010 12:04, schrieb address@hidden:
>> +
>> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>> +    if (backing_fmt && backing_fmt->value.s) {
>> +        if (!bdrv_find_format(backing_fmt->value.s)) {
>> +            error_report("Unknown backing file format '%s'",
>> +                         backing_fmt->value.s);
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    if (base_fmt) {
>> +        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> +            error_report("Backing file format not supported for file "
>> +                         "format '%s'", fmt);
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +    }
> 
> The order is wrong here. If you use -F, the backing format won't be checked.

Urgh yes, my bad! I had it the other way, but decided to change it -
very silly.

>> +
>> +    // The size for the image must always be specified, with one exception:
>> +    // If we are using a backing file, we can obtain the size from there
>> +    if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) {
>> +        QEMUOptionParameter *backing_file =
>> +            get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>> +
>> +        if (backing_file && backing_file->value.s) {
>> +            uint64_t size;
>> +            const char *fmt = NULL;
>> +            char buf[32];
>> +
>> +            if (backing_fmt && backing_fmt->value.s) {
>> +                fmt = backing_fmt->value.s;
>> +            }
>> +
>> +            bs = bdrv_new("");
>> +            if (!bs) {
>> +                error_report("Not enough memory to allocate 
>> BlockDriverState");
>> +                ret = -1;
>> +                goto out;
>> +            }
> 
> bdrv_new never returns NULL (it's an indirect qemu_malloc call).

I see - this was copied wholesale from qemu-img.c but I'll just remove
the error check.

>> +            ret = bdrv_open(bs, backing_file->value.s, flags, drv);
>> +            if (ret < 0) {
>> +                error_report("Could not open '%s'", filename);
>> +                ret = -1;
>> +                goto out;
>> +            }
>> +            bdrv_get_geometry(bs, &size);
>> +            size *= 512;
>> +
>> +            snprintf(buf, sizeof(buf), "%" PRId64, size);
>> +            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
>> +        } else {
>> +            error_report("Image creation needs a size parameter");
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    printf("Formatting '%s', fmt=%s ", filename, fmt);
>> +    print_option_parameters(param);
>> +    puts("");
>> +
>> +    ret = bdrv_create(drv, filename, param);
>> +    free_option_parameters(create_options);
>> +    free_option_parameters(param);
> 
> These need to be after out: to avoid leaking in error cases.
> 
> You're basically reverting a87a6721d with this.

Whoops - another one of those conflicting ones. It's all Stefan's fault :)

>> +
>> +    if (ret < 0) {
>> +        if (ret == -ENOTSUP) {
>> +            error_report("Formatting or formatting option not supported for 
>> "
>> +                         "file format '%s'", fmt);
>> +        } else if (ret == -EFBIG) {
>> +            error_report("The image size is too large for file format '%s'",
>> +                         fmt);
>> +        } else {
>> +            error_report("%s: error while creating %s: %s", filename, fmt,
>> +                         strerror(-ret));
>> +        }
>> +    }
>> +
>> +out:
>> +    if (bs) {
>> +        bdrv_delete(bs);
>> +    }
>> +    if (ret) {
>> +        return 1;
>> +    }
> 
> Maybe we should better use the usual 0/-errno style. In qemu-img it was
> the exit code of the program, but now it's a block layer function.

I like errno, I made it behave like this for consistency with the rest
of QEMU. In most places we don't seem to like anything but -1/1 for
error values.

Let me know what you prefer, or alternatively we can change it in a
follow-on patch?

Cheers,
Jes