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

[Kevin Wolf]

Am 12.11.2010 17:34, schrieb Stefan Hajnoczi:
> 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?

That's how it's meant to be used. I think at the moment we still allow
to directly use file etc. and you know that if something is possible,
users will do it.

But instead of listing all of them here, maybe we should just make the
final step and return an error if they are used directly. Unfortunately,
that will bring us back to the discussion about formats and 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.

Okay, that makes sense.

Kevin