qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: [PATCH v4 2/5] qed: Add QEMU Enhanced Disk image fo


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] Re: [PATCH v4 2/5] qed: Add QEMU Enhanced Disk image format
Date: Fri, 12 Nov 2010 16:34:42 +0000

On Fri, Nov 12, 2010 at 3:43 PM, Kevin Wolf <address@hidden> wrote:
> Am 28.10.2010 13:01, schrieb Stefan Hajnoczi:
>> +/**
>> + * Check whether an image format is raw
>> + *
>> + * @fmt:    Backing file format, may be NULL
>> + */
>> +static bool qed_fmt_is_raw(const char *fmt)
>> +{
>> +    return fmt && strcmp(fmt, "raw") == 0;
>> +}
>
> People shouldn't use them directly, but should we also consider file,
> host_device, etc.?

Hrm..I will look into it for v5.  I thought we always have a "raw"
format on top of "file", "host_device", etc protocols?

>> +    if (s->header.magic != QED_MAGIC) {
>> +        return -ENOENT;
>> +    }
>
> ENOENT seems a bit odd for a wrong magic number, especially if it's used
> for the error message. Wouldn't EINVAL or ENOTSUP be a closer match?

You're right, ENOENT is confusing for the user.

>> +static void bdrv_qed_flush(BlockDriverState *bs)
>> +{
>> +    bdrv_flush(bs->file);
>> +}
>
> This conflicts with one of my recent changes. bdrv_flush should return
> an int now.

Will fix and will also check for bdrv_flush() failures.

>> +    while (options && options->name) {
>> +        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> +            image_size = options->value.n;
>> +        } 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;
>
> I'm not sure here. It doesn't really matter for QED, but I find it
> strange that -o backing_fmt=foobar works and gives you a non-raw backing
> file. Should we check if the format exists at least?

I see the same issue in QCOW2 so let's solve this generically in a
separate patch.

Stefan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]