[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 1/3] qemu-img.c: Re-factor img_create()
From: |
Jes Sorensen |
Subject: |
[Qemu-devel] Re: [PATCH 1/3] qemu-img.c: Re-factor img_create() |
Date: |
Thu, 16 Dec 2010 13:05:01 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.6 |
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