On Fri, Nov 12, 2010 at 3:43 PM, Kevin Wolf <kw...@redhat.com> 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
