[Qemu-devel] [PATCH 0/4] block: Fix error report for wrong file format

[Qemu-devel] [PATCH 0/4] block: Fix error report for wrong file format

[Stefan Weil]

These patches improve the error report if the file format was
specified explicitly (example: -drive file=myfile,format=qcow2)
and the given format does not match the real format.

This fixes those bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=556482
https://bugs.launchpad.net/qemu/+bug/1090600

[PATCH 1/4] block: Add special error code for wrong format
[PATCH 2/4] block: Improve error report for wrong format
[PATCH 3/4] block: Use new error code for wrong format in selected
[PATCH 4/4] block/vdi: Improved return values from vdi_open and

[Qemu-devel] [PATCH 1/4] block: Add special error code for wrong format

[Stefan Weil]

The block drivers normally return -errno for typical errors.
There is no appropriate error code for "wrong format", so
use a special error code which does not conflict with system
error codes.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 block.h |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.h b/block.h
index 893448a..829e18b 100644
--- a/block.h
+++ b/block.h
@@ -90,6 +90,13 @@ typedef struct BlockDevOps {
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
+/* The block drivers normally return -errno for typical errors.
+ * There is no appropriate error code for "wrong format", so
+ * use a special error code which does not conflict with system
+ * error codes.
+ */
+#define BDRV_WRONG_FORMAT  INT_MIN
+
 typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockErrorAction;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] block: Add special error code for wrong format

[Stefan Hajnoczi]

On Sat, Dec 15, 2012 at 03:09:30PM +0100, Stefan Weil wrote:
> The block drivers normally return -errno for typical errors.
> There is no appropriate error code for "wrong format", so
> use a special error code which does not conflict with system
> error codes.

ENOTTY is used when something is of the wrong type.  Since the name
"ENOTTY" is not clear, defining a new error code makes sense though.

Stefan

Re: [Qemu-devel] [PATCH 1/4] block: Add special error code for wrong format

[Kevin Wolf]

Am 15.12.2012 15:09, schrieb Stefan Weil:
> The block drivers normally return -errno for typical errors.
> There is no appropriate error code for "wrong format", so
> use a special error code which does not conflict with system
> error codes.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  block.h |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block.h b/block.h
> index 893448a..829e18b 100644
> --- a/block.h
> +++ b/block.h
> @@ -90,6 +90,13 @@ typedef struct BlockDevOps {
>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>  
> +/* The block drivers normally return -errno for typical errors.
> + * There is no appropriate error code for "wrong format", so
> + * use a special error code which does not conflict with system
> + * error codes.
> + */
> +#define BDRV_WRONG_FORMAT  INT_MIN

I think it would be better to use the E* format and a positive number so
that it's obvious that it's meant to be used in -errno returns.

Also, I would consider moving it to qemu-common.h where other errno
values are defined that may be missing on some systems, so that
everything stays in one place and we won't define overlapping codes:

#if !defined(ENOTSUP)
#define ENOTSUP 4096
#endif
#if !defined(ECANCELED)
#define ECANCELED 4097
#endif

This sounds like a good addition in the same place would be:

#define EBDRV_WRONG_FORMAT 4098

Or just use EINVAL or ENOTTY like Stefan suggested.

Kevin

[Qemu-devel] [PATCH 2/4] block: Improve error report for wrong format

[Stefan Weil]

There is no good system error for this kind of error,
so it needs special handling instead of strerror.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 blockdev.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5a4cd56..3da44f6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -624,8 +624,13 @@ DriveInfo *drive_init(QemuOpts *opts, BlockInterfaceType block_default_type)
 
     ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
     if (ret < 0) {
-        error_report("could not open disk image %s: %s",
-                     file, strerror(-ret));
+        if (ret == BDRV_WRONG_FORMAT) {
+            error_report("could not open disk image %s: not in %s format",
+                         file, drv->format_name);
+        } else {
+            error_report("could not open disk image %s: %s",
+                         file, strerror(-ret));
+        }
         goto err;
     }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/4] block: Improve error report for wrong format

[Luiz Capitulino]

On Sat, 15 Dec 2012 15:09:31 +0100
Stefan Weil <sw@weilnetz.de> wrote:

> There is no good system error for this kind of error,
> so it needs special handling instead of strerror.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  blockdev.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5a4cd56..3da44f6 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -624,8 +624,13 @@ DriveInfo *drive_init(QemuOpts *opts, BlockInterfaceType block_default_type)
>  
>      ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
>      if (ret < 0) {
> -        error_report("could not open disk image %s: %s",
> -                     file, strerror(-ret));
> +        if (ret == BDRV_WRONG_FORMAT) {
> +            error_report("could not open disk image %s: not in %s format",
> +                         file, drv->format_name);
> +        } else {
> +            error_report("could not open disk image %s: %s",
> +                         file, strerror(-ret));

IIRC, I have an rfc series propagating an Error object down to bdrv_open(),
and was planning to propagate it to block drivers. You could do it instead
of creating this new error code.

It's usually more work, but the end result is usually better.

> +        }
>          goto err;
>      }
>  

[Qemu-devel] [PATCH 3/4] block: Use new error code for wrong format in selected block drivers

[Stefan Weil]

This improves error reports for bochs, cow, qcow, qcow2, qed and vmdk
when a file with the wrong format is selected.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 block/bochs.c |    2 +-
 block/cow.c   |    2 +-
 block/qcow.c  |    2 +-
 block/qcow2.c |    2 +-
 block/qed.c   |    2 +-
 block/vmdk.c  |    4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index ab7944d..063dc3f 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -126,7 +126,7 @@ static int bochs_open(BlockDriverState *bs, int flags)
         strcmp(bochs.subtype, GROWING_TYPE) ||
 	((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
 	(le32_to_cpu(bochs.version) != HEADER_V1))) {
-        goto fail;
+        return BDRV_WRONG_FORMAT;
     }
 
     if (le32_to_cpu(bochs.version) == HEADER_V1) {
diff --git a/block/cow.c b/block/cow.c
index a5a00eb..7ae5ddc 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -73,7 +73,7 @@ static int cow_open(BlockDriverState *bs, int flags)
     }
 
     if (be32_to_cpu(cow_header.magic) != COW_MAGIC) {
-        ret = -EINVAL;
+        ret = BDRV_WRONG_FORMAT;
         goto fail;
     }
 
diff --git a/block/qcow.c b/block/qcow.c
index b239c82..d1be009 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -112,7 +112,7 @@ static int qcow_open(BlockDriverState *bs, int flags)
     be64_to_cpus(&header.l1_table_offset);
 
     if (header.magic != QCOW_MAGIC) {
-        ret = -EINVAL;
+        ret = BDRV_WRONG_FORMAT;
         goto fail;
     }
     if (header.version != QCOW_VERSION) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 8520bda..f22e387 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -311,7 +311,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     be32_to_cpus(&header.nb_snapshots);
 
     if (header.magic != QCOW_MAGIC) {
-        ret = -EINVAL;
+        ret = BDRV_WRONG_FORMAT;
         goto fail;
     }
     if (header.version < 2 || header.version > 3) {
diff --git a/block/qed.c b/block/qed.c
index 0b5374a..58bd35c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -390,7 +390,7 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
     qed_header_le_to_cpu(&le_header, &s->header);
 
     if (s->header.magic != QED_MAGIC) {
-        return -EINVAL;
+        return BDRV_WRONG_FORMAT;
     }
     if (s->header.features & ~QED_FEATURE_MASK) {
         /* image uses unsupported feature bits */
diff --git a/block/vmdk.c b/block/vmdk.c
index 51398c0..931ffbc 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -616,7 +616,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
             return vmdk_open_vmdk4(bs, file, flags);
             break;
         default:
-            return -EINVAL;
+            return BDRV_WRONG_FORMAT;
             break;
     }
 }
@@ -718,7 +718,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
     }
     buf[2047] = '\0';
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
-        return -EINVAL;
+        return BDRV_WRONG_FORMAT;
     }
     if (strcmp(ct, "monolithicFlat") &&
         strcmp(ct, "twoGbMaxExtentSparse") &&
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/4] block: Use new error code for wrong format in selected block drivers

[Don Slutz]

On 12/15/12 09:09, Stefan Weil wrote:
> This improves error reports for bochs, cow, qcow, qcow2, qed and vmdk
> when a file with the wrong format is selected.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>   block/bochs.c |    2 +-
>   block/cow.c   |    2 +-
>   block/qcow.c  |    2 +-
>   block/qcow2.c |    2 +-
>   block/qed.c   |    2 +-
>   block/vmdk.c  |    4 ++--
>   6 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/block/bochs.c b/block/bochs.c
> index ab7944d..063dc3f 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -126,7 +126,7 @@ static int bochs_open(BlockDriverState *bs, int flags)
>           strcmp(bochs.subtype, GROWING_TYPE) ||
>   	((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
>   	(le32_to_cpu(bochs.version) != HEADER_V1))) {
> -        goto fail;
> +        return BDRV_WRONG_FORMAT;
As far as I can tell, all "goto fail" are this error, and so the
  fail:
     return -1;
Is what should be changed.
>       }
>   
>       if (le32_to_cpu(bochs.version) == HEADER_V1) {
> diff --git a/block/cow.c b/block/cow.c
> index a5a00eb..7ae5ddc 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -73,7 +73,7 @@ static int cow_open(BlockDriverState *bs, int flags)
>       }
>   
>       if (be32_to_cpu(cow_header.magic) != COW_MAGIC) {
> -        ret = -EINVAL;
> +        ret = BDRV_WRONG_FORMAT;
>           goto fail;
>       }
>   
> diff --git a/block/qcow.c b/block/qcow.c
> index b239c82..d1be009 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -112,7 +112,7 @@ static int qcow_open(BlockDriverState *bs, int flags)
>       be64_to_cpus(&header.l1_table_offset);
>   
>       if (header.magic != QCOW_MAGIC) {
> -        ret = -EINVAL;
> +        ret = BDRV_WRONG_FORMAT;
>           goto fail;
>       }
>       if (header.version != QCOW_VERSION) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8520bda..f22e387 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -311,7 +311,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
>       be32_to_cpus(&header.nb_snapshots);
>   
>       if (header.magic != QCOW_MAGIC) {
> -        ret = -EINVAL;
> +        ret = BDRV_WRONG_FORMAT;
>           goto fail;
>       }
>       if (header.version < 2 || header.version > 3) {
> diff --git a/block/qed.c b/block/qed.c
> index 0b5374a..58bd35c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -390,7 +390,7 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>       qed_header_le_to_cpu(&le_header, &s->header);
>   
>       if (s->header.magic != QED_MAGIC) {
> -        return -EINVAL;
> +        return BDRV_WRONG_FORMAT;
>       }
>       if (s->header.features & ~QED_FEATURE_MASK) {
>           /* image uses unsupported feature bits */
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 51398c0..931ffbc 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -616,7 +616,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
>               return vmdk_open_vmdk4(bs, file, flags);
>               break;
>           default:
> -            return -EINVAL;
> +            return BDRV_WRONG_FORMAT;
>               break;
>       }
>   }
> @@ -718,7 +718,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
>       }
>       buf[2047] = '\0';
>       if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> -        return -EINVAL;
> +        return BDRV_WRONG_FORMAT;
>       }
>       if (strcmp(ct, "monolithicFlat") &&
>           strcmp(ct, "twoGbMaxExtentSparse") &&
    -Don Slutz

[Qemu-devel] [PATCH 4/4] block/vdi: Improved return values from vdi_open and other small fixes

[Stefan Weil]

vdi_open returned -1 in case of any error, but it should return an
error code (negative value of errno or BDRV_WRONG_FORMAT).

vdi_open did not check for a bad signature. This check was only in vdi_probe.

The signature is a 32 bit value and needs up to 8 hex digits for printing.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 block/vdi.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index c8330b7..7c7699f 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -246,7 +246,7 @@ static void vdi_header_print(VdiHeader *header)
 {
     char uuid[37];
     logout("text        %s", header->text);
-    logout("signature   0x%04x\n", header->signature);
+    logout("signature   0x%08x\n", header->signature);
     logout("header size 0x%04x\n", header->header_size);
     logout("image type  0x%04x\n", header->image_type);
     logout("image flags 0x%04x\n", header->image_flags);
@@ -369,10 +369,12 @@ static int vdi_open(BlockDriverState *bs, int flags)
     BDRVVdiState *s = bs->opaque;
     VdiHeader header;
     size_t bmap_size;
+    int ret;
 
     logout("\n");
 
-    if (bdrv_read(bs->file, 0, (uint8_t *)&header, 1) < 0) {
+    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
+    if (ret < 0) {
         goto fail;
     }
 
@@ -390,33 +392,45 @@ static int vdi_open(BlockDriverState *bs, int flags)
         header.disk_size &= ~(SECTOR_SIZE - 1);
     }
 
-    if (header.version != VDI_VERSION_1_1) {
+    if (header.signature != VDI_SIGNATURE) {
+        logout("bad vdi signature %08x\n", header.signature);
+        ret = BDRV_WRONG_FORMAT;
+        goto fail;
+    } else if (header.version != VDI_VERSION_1_1) {
         logout("unsupported version %u.%u\n",
                header.version >> 16, header.version & 0xffff);
+        ret = -ENOTSUP;
         goto fail;
     } else if (header.offset_bmap % SECTOR_SIZE != 0) {
         /* We only support block maps which start on a sector boundary. */
         logout("unsupported block map offset 0x%x B\n", header.offset_bmap);
+        ret = -ENOTSUP;
         goto fail;
     } else if (header.offset_data % SECTOR_SIZE != 0) {
         /* We only support data blocks which start on a sector boundary. */
         logout("unsupported data offset 0x%x B\n", header.offset_data);
+        ret = -ENOTSUP;
         goto fail;
     } else if (header.sector_size != SECTOR_SIZE) {
         logout("unsupported sector size %u B\n", header.sector_size);
+        ret = -ENOTSUP;
         goto fail;
     } else if (header.block_size != 1 * MiB) {
         logout("unsupported block size %u B\n", header.block_size);
+        ret = -ENOTSUP;
         goto fail;
     } else if (header.disk_size >
                (uint64_t)header.blocks_in_image * header.block_size) {
         logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
+        ret = -ENOTSUP;
         goto fail;
     } else if (!uuid_is_null(header.uuid_link)) {
         logout("link uuid != 0, unsupported\n");
+        ret = -ENOTSUP;
         goto fail;
     } else if (!uuid_is_null(header.uuid_parent)) {
         logout("parent uuid != 0, unsupported\n");
+        ret = -ENOTSUP;
         goto fail;
     }
 
@@ -432,7 +446,8 @@ static int vdi_open(BlockDriverState *bs, int flags)
     if (bmap_size > 0) {
         s->bmap = g_malloc(bmap_size * SECTOR_SIZE);
     }
-    if (bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap, bmap_size) < 0) {
+    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap, bmap_size);
+    if (ret < 0) {
         goto fail_free_bmap;
     }
 
@@ -448,7 +463,7 @@ static int vdi_open(BlockDriverState *bs, int flags)
     g_free(s->bmap);
 
  fail:
-    return -1;
+    return ret;
 }
 
 static int vdi_reopen_prepare(BDRVReopenState *state,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] block/vdi: Improved return values from vdi_open and other small fixes

[Kevin Wolf]

Am 15.12.2012 15:09, schrieb Stefan Weil:
> vdi_open returned -1 in case of any error, but it should return an
> error code (negative value of errno or BDRV_WRONG_FORMAT).
> 
> vdi_open did not check for a bad signature. This check was only in vdi_probe.
> 
> The signature is a 32 bit value and needs up to 8 hex digits for printing.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Sounds like three independent changes and should be three patches therefore.

Kevin

Re: [Qemu-devel] [PATCH 0/4] block: Fix error report for wrong file format

[Stefan Weil]

Am 15.12.2012 15:09, schrieb Stefan Weil:
> These patches improve the error report if the file format was
> specified explicitly (example: -drive file=myfile,format=qcow2)
> and the given format does not match the real format.
>
> This fixes those bugs:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=556482
> https://bugs.launchpad.net/qemu/+bug/1090600
>
> [PATCH 1/4] block: Add special error code for wrong format
> [PATCH 2/4] block: Improve error report for wrong format
> [PATCH 3/4] block: Use new error code for wrong format in selected
> [PATCH 4/4] block/vdi: Improved return values from vdi_open and

Hi Stefan und Kevin,

these patches are still in my local queue.

Do you plan to add them to the block queue, or would
you prefer another solution for the open bug reports?

Regards

Stefan

Re: [Qemu-devel] [PATCH 0/4] block: Fix error report for wrong file format

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 07:53:35PM +0100, Stefan Weil wrote:
> Am 15.12.2012 15:09, schrieb Stefan Weil:
> >These patches improve the error report if the file format was
> >specified explicitly (example: -drive file=myfile,format=qcow2)
> >and the given format does not match the real format.
> >
> >This fixes those bugs:
> >
> >https://bugzilla.redhat.com/show_bug.cgi?id=556482
> >https://bugs.launchpad.net/qemu/+bug/1090600
> >
> >[PATCH 1/4] block: Add special error code for wrong format
> >[PATCH 2/4] block: Improve error report for wrong format
> >[PATCH 3/4] block: Use new error code for wrong format in selected
> >[PATCH 4/4] block/vdi: Improved return values from vdi_open and
> 
> Hi Stefan und Kevin,
> 
> these patches are still in my local queue.
> 
> Do you plan to add them to the block queue, or would
> you prefer another solution for the open bug reports?

Looks okay to me.  I'm not thrilled about introducing a non-system error
code, would have rather have used EINVAL or ENOTTY.  But that's not a
killer and I see the reason you chose to do that.

Kevin: Any comments before I merge this?

Stefan

Re: [Qemu-devel] [PATCH 0/4] block: Fix error report for wrong file format

[Kevin Wolf]

Am 17.01.2013 09:33, schrieb Stefan Hajnoczi:
> On Wed, Jan 16, 2013 at 07:53:35PM +0100, Stefan Weil wrote:
>> Am 15.12.2012 15:09, schrieb Stefan Weil:
>>> These patches improve the error report if the file format was
>>> specified explicitly (example: -drive file=myfile,format=qcow2)
>>> and the given format does not match the real format.
>>>
>>> This fixes those bugs:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=556482
>>> https://bugs.launchpad.net/qemu/+bug/1090600
>>>
>>> [PATCH 1/4] block: Add special error code for wrong format
>>> [PATCH 2/4] block: Improve error report for wrong format
>>> [PATCH 3/4] block: Use new error code for wrong format in selected
>>> [PATCH 4/4] block/vdi: Improved return values from vdi_open and
>>
>> Hi Stefan und Kevin,
>>
>> these patches are still in my local queue.
>>
>> Do you plan to add them to the block queue, or would
>> you prefer another solution for the open bug reports?
> 
> Looks okay to me.  I'm not thrilled about introducing a non-system error
> code, would have rather have used EINVAL or ENOTTY.  But that's not a
> killer and I see the reason you chose to do that.
> 
> Kevin: Any comments before I merge this?

Yes, I commented on the patches. I think the minimum that should change
is moving the error code definition to where other error codes are
defined in order to avoid future collisions.

Ideally we'd convert bdrv_open to Error and avoid all this error code
stuff, but I'm not requesting this now.

Kevin