[Qemu-devel] [PATCH 0.14/master 0/4] Error messages for unsupoorted image format features

[Qemu-devel] [PATCH 0.14/master 0/4] Error messages for unsupoorted image format features

[Kevin Wolf]

With 0.15 we'll most likely get some incompatible image format extensions. This
series prepares 0.14 to output more helpful messages if it stumbles over a too
new image file.

Kevin Wolf (4):
  qerror: Add QERR_UNKNOWN_BLOCK_FORMAT_FEATURE
  qcow2: Report error for version > 2
  qed: Report error for unsupported features
  qemu-img: Improve error messages for failed bdrv_open

 block/qcow2.c |   13 +++++++++++--
 block/qed.c   |    8 +++++++-
 qemu-img.c    |   10 +++++++---
 qerror.c      |    5 +++++
 qerror.h      |    3 +++
 5 files changed, 33 insertions(+), 6 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/4] qerror: Add QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qerror.c |    5 +++++
 qerror.h |    3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 9d0cdeb..62dcc1a 100644
--- a/qerror.c
+++ b/qerror.c
@@ -201,6 +201,11 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "An undefined error has ocurred",
     },
     {
+        .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+        .desc      = "'%(device)' uses an image format feature which is not "
+                     "supported by this qemu version: %(feature)",
+    },
+    {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
diff --git a/qerror.h b/qerror.h
index b0f69da..31d6df3 100644
--- a/qerror.h
+++ b/qerror.h
@@ -165,6 +165,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
+#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
+    "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'feature': %s } }"
+
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] qcow2: Report error for version > 2

[Kevin Wolf]

The qcow2 driver is now declared responsible for any QCOW image that has
version 2 or greater (before this, version 3 would be detected as raw).

For everything newer than version 2, an error is reported.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 551b3c2..7abd8be 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -28,6 +28,7 @@
 #include "aes.h"
 #include "block/qcow2.h"
 #include "qemu-error.h"
+#include "qerror.h"
 
 /*
   Differences with QCOW:
@@ -59,7 +60,7 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 
     if (buf_size >= sizeof(QCowHeader) &&
         be32_to_cpu(cow_header->magic) == QCOW_MAGIC &&
-        be32_to_cpu(cow_header->version) == QCOW_VERSION)
+        be32_to_cpu(cow_header->version) >= QCOW_VERSION)
         return 100;
     else
         return 0;
@@ -163,10 +164,18 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     be64_to_cpus(&header.snapshots_offset);
     be32_to_cpus(&header.nb_snapshots);
 
-    if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION) {
+    if (header.magic != QCOW_MAGIC) {
         ret = -EINVAL;
         goto fail;
     }
+    if (header.version != QCOW_VERSION) {
+        char version[64];
+        snprintf(version, sizeof(version), "QCOW version %d", header.version);
+        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+            bs->device_name, version);
+        ret = -ENOTSUP;
+        goto fail;
+    }
     if (header.cluster_bits < MIN_CLUSTER_BITS ||
         header.cluster_bits > MAX_CLUSTER_BITS) {
         ret = -EINVAL;
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/4] qed: Report error for unsupported features

[Kevin Wolf]

Instead of just returning -ENOTSUP, generate a more detailed error.

Unfortunately we don't have a helpful text for features that we don't know yet,
so just print the feature mask. It might be useful at least if someone asks for
help.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 3273448..8b0a975 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -14,6 +14,7 @@
 
 #include "trace.h"
 #include "qed.h"
+#include "qerror.h"
 
 static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
 {
@@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
         return -EINVAL;
     }
     if (s->header.features & ~QED_FEATURE_MASK) {
-        return -ENOTSUP; /* image uses unsupported feature bits */
+        /* image uses unsupported feature bits */
+        char version[64];
+        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
+        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+            bs->device_name, version);
+        return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
         return -EINVAL;
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] qemu-img: Improve error messages for failed bdrv_open

[Kevin Wolf]

Output the error message string of the bdrv_open return code. Also set a
non-empty device name for the images because the unknown feature error message
includes it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 4a37358..7e3cc4c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -213,8 +213,9 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     BlockDriverState *bs;
     BlockDriver *drv;
     char password[256];
+    int ret;
 
-    bs = bdrv_new("");
+    bs = bdrv_new("image");
 
     if (fmt) {
         drv = bdrv_find_format(fmt);
@@ -225,10 +226,13 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     } else {
         drv = NULL;
     }
-    if (bdrv_open(bs, filename, flags, drv) < 0) {
-        error_report("Could not open '%s'", filename);
+
+    ret = bdrv_open(bs, filename, flags, drv);
+    if (ret < 0) {
+        error_report("Could not open '%s': %s", filename, strerror(-ret));
         goto fail;
     }
+
     if (bdrv_is_encrypted(bs)) {
         printf("Disk image '%s' is encrypted.\n", filename);
         if (read_password(password, sizeof(password)) < 0) {
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 1/4] qerror: Add QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Anthony Liguori]

On 02/09/2011 04:31 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   qerror.c |    5 +++++
>   qerror.h |    3 +++
>   2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/qerror.c b/qerror.c
> index 9d0cdeb..62dcc1a 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -201,6 +201,11 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "An undefined error has ocurred",
>       },
>       {
> +        .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> +        .desc      = "'%(device)' uses an image format feature which is not "
> +                     "supported by this qemu version: %(feature)",
> +    },
> +    {
>           .error_fmt = QERR_VNC_SERVER_FAILED,
>           .desc      = "Could not start VNC server on %(target)",
>       },
> diff --git a/qerror.h b/qerror.h
> index b0f69da..31d6df3 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -165,6 +165,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_UNDEFINED_ERROR \
>       "{ 'class': 'UndefinedError', 'data': {} }"
>
> +#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
> +    "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'feature': %s } }"
>    

Would be good to include the name of the block format in the error 
message to put the feature in context.

Regards,

Anthony Liguori

>   #define QERR_VNC_SERVER_FAILED \
>       "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>
>    

[Qemu-devel] Re: [PATCH 3/4] qed: Report error for unsupported features

[Anthony Liguori]

On 02/09/2011 04:32 AM, Kevin Wolf wrote:
> Instead of just returning -ENOTSUP, generate a more detailed error.
>
> Unfortunately we don't have a helpful text for features that we don't know yet,
> so just print the feature mask. It might be useful at least if someone asks for
> help.
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>    

We can use a compatible feature to create a feature name table.

> ---
>   block/qed.c |    8 +++++++-
>   1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index 3273448..8b0a975 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -14,6 +14,7 @@
>
>   #include "trace.h"
>   #include "qed.h"
> +#include "qerror.h"
>
>   static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
>   {
> @@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>           return -EINVAL;
>       }
>       if (s->header.features&  ~QED_FEATURE_MASK) {
> -        return -ENOTSUP; /* image uses unsupported feature bits */
> +        /* image uses unsupported feature bits */
> +        char version[64];
> +        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
>    

It would be useful to do s->header.features & QED_FEATURE_MASK here as a 
management tool doesn't know what features this version of QED supports.

Regards,

Anthony Liguori

> +        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> +            bs->device_name, version);
> +        return -ENOTSUP;
>       }
>       if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
>           return -EINVAL;
>    

[Qemu-devel] Re: [PATCH 3/4] qed: Report error for unsupported features

[Anthony Liguori]

On 02/09/2011 04:32 AM, Kevin Wolf wrote:
> Instead of just returning -ENOTSUP, generate a more detailed error.
>
> Unfortunately we don't have a helpful text for features that we don't know yet,
> so just print the feature mask. It might be useful at least if someone asks for
> help.
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>    

We can use a compatible feature to create a feature name table.

> ---
>   block/qed.c |    8 +++++++-
>   1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index 3273448..8b0a975 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -14,6 +14,7 @@
>
>   #include "trace.h"
>   #include "qed.h"
> +#include "qerror.h"
>
>   static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
>   {
> @@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>           return -EINVAL;
>       }
>       if (s->header.features&  ~QED_FEATURE_MASK) {
> -        return -ENOTSUP; /* image uses unsupported feature bits */
> +        /* image uses unsupported feature bits */
> +        char version[64];
> +        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
>    

It would be useful to do s->header.features & QED_FEATURE_MASK here as a 
management tool doesn't know what features this version of QED supports.

Regards,

Anthony Liguori

> +        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> +            bs->device_name, version);
> +        return -ENOTSUP;
>       }
>       if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
>           return -EINVAL;
>    

[Qemu-devel] Re: [PATCH 1/4] qerror: Add QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Kevin Wolf]

Am 09.02.2011 12:08, schrieb Anthony Liguori:
> On 02/09/2011 04:31 AM, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>> ---
>>   qerror.c |    5 +++++
>>   qerror.h |    3 +++
>>   2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/qerror.c b/qerror.c
>> index 9d0cdeb..62dcc1a 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -201,6 +201,11 @@ static const QErrorStringTable qerror_table[] = {
>>           .desc      = "An undefined error has ocurred",
>>       },
>>       {
>> +        .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> +        .desc      = "'%(device)' uses an image format feature which is not "
>> +                     "supported by this qemu version: %(feature)",
>> +    },
>> +    {
>>           .error_fmt = QERR_VNC_SERVER_FAILED,
>>           .desc      = "Could not start VNC server on %(target)",
>>       },
>> diff --git a/qerror.h b/qerror.h
>> index b0f69da..31d6df3 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -165,6 +165,9 @@ QError *qobject_to_qerror(const QObject *obj);
>>   #define QERR_UNDEFINED_ERROR \
>>       "{ 'class': 'UndefinedError', 'data': {} }"
>>
>> +#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
>> +    "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'feature': %s } }"
>>    
> 
> Would be good to include the name of the block format in the error 
> message to put the feature in context.

I actually had the format name there initally, but then I replaced it
with the device name because I thought that would be more helpful. If
you prefer, I can add it back so that we have both.

Kevin

[Qemu-devel] Re: [PATCH 1/4] qerror: Add QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Anthony Liguori]

On 02/09/2011 05:26 AM, Kevin Wolf wrote:
> Am 09.02.2011 12:08, schrieb Anthony Liguori:
>    
>> On 02/09/2011 04:31 AM, Kevin Wolf wrote:
>>      
>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>> ---
>>>    qerror.c |    5 +++++
>>>    qerror.h |    3 +++
>>>    2 files changed, 8 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qerror.c b/qerror.c
>>> index 9d0cdeb..62dcc1a 100644
>>> --- a/qerror.c
>>> +++ b/qerror.c
>>> @@ -201,6 +201,11 @@ static const QErrorStringTable qerror_table[] = {
>>>            .desc      = "An undefined error has ocurred",
>>>        },
>>>        {
>>> +        .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>>> +        .desc      = "'%(device)' uses an image format feature which is not "
>>> +                     "supported by this qemu version: %(feature)",
>>> +    },
>>> +    {
>>>            .error_fmt = QERR_VNC_SERVER_FAILED,
>>>            .desc      = "Could not start VNC server on %(target)",
>>>        },
>>> diff --git a/qerror.h b/qerror.h
>>> index b0f69da..31d6df3 100644
>>> --- a/qerror.h
>>> +++ b/qerror.h
>>> @@ -165,6 +165,9 @@ QError *qobject_to_qerror(const QObject *obj);
>>>    #define QERR_UNDEFINED_ERROR \
>>>        "{ 'class': 'UndefinedError', 'data': {} }"
>>>
>>> +#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
>>> +    "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'feature': %s } }"
>>>
>>>        
>> Would be good to include the name of the block format in the error
>> message to put the feature in context.
>>      
> I actually had the format name there initally, but then I replaced it
> with the device name because I thought that would be more helpful. If
> you prefer, I can add it back so that we have both.
>    

Yes, I think both would be best.

If I'm going to display this error to a user, I want to say something 
like "Could not start machine because this image uses the 'free list' 
feature of 'qed' which is not supported by this version of QEMU, please 
upgrade."

With the device name, I can do a query-block to figure out the format 
but it's nice to avoid the extra round trip.

Regards,

Anthony Liguori

> Kevin
>    

[Qemu-devel] Re: [PATCH 3/4] qed: Report error for unsupported features

[Kevin Wolf]

Am 09.02.2011 12:21, schrieb Anthony Liguori:
> On 02/09/2011 04:32 AM, Kevin Wolf wrote:
>> Instead of just returning -ENOTSUP, generate a more detailed error.
>>
>> Unfortunately we don't have a helpful text for features that we don't know yet,
>> so just print the feature mask. It might be useful at least if someone asks for
>> help.
>>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>    
> 
> We can use a compatible feature to create a feature name table.

If you want to introduce something like this in a 0.14-rc2...? I'm not
going to write the code, but if someone sends a patch, we can consider it.

>> ---
>>   block/qed.c |    8 +++++++-
>>   1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/qed.c b/block/qed.c
>> index 3273448..8b0a975 100644
>> --- a/block/qed.c
>> +++ b/block/qed.c
>> @@ -14,6 +14,7 @@
>>
>>   #include "trace.h"
>>   #include "qed.h"
>> +#include "qerror.h"
>>
>>   static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
>>   {
>> @@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>>           return -EINVAL;
>>       }
>>       if (s->header.features&  ~QED_FEATURE_MASK) {
>> -        return -ENOTSUP; /* image uses unsupported feature bits */
>> +        /* image uses unsupported feature bits */
>> +        char version[64];
>> +        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
>>    
> 
> It would be useful to do s->header.features & QED_FEATURE_MASK here as a 
> management tool doesn't know what features this version of QED supports.

Makes sense. I'll change this for v2.

Kevin

[Qemu-devel] Re: [PATCH 3/4] qed: Report error for unsupported features

[Anthony Liguori]

On 02/09/2011 05:29 AM, Kevin Wolf wrote:
> Am 09.02.2011 12:21, schrieb Anthony Liguori:
>    
>> On 02/09/2011 04:32 AM, Kevin Wolf wrote:
>>      
>>> Instead of just returning -ENOTSUP, generate a more detailed error.
>>>
>>> Unfortunately we don't have a helpful text for features that we don't know yet,
>>> so just print the feature mask. It might be useful at least if someone asks for
>>> help.
>>>
>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>>
>>>        
>> We can use a compatible feature to create a feature name table.
>>      
> If you want to introduce something like this in a 0.14-rc2...? I'm not
> going to write the code, but if someone sends a patch, we can consider it.
>    

Not for 0.14-rc2 but for 0.15.0.  Was just mentioning it as an idea.

Regards,

Anthony Liguori

>    
>>> ---
>>>    block/qed.c |    8 +++++++-
>>>    1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/block/qed.c b/block/qed.c
>>> index 3273448..8b0a975 100644
>>> --- a/block/qed.c
>>> +++ b/block/qed.c
>>> @@ -14,6 +14,7 @@
>>>
>>>    #include "trace.h"
>>>    #include "qed.h"
>>> +#include "qerror.h"
>>>
>>>    static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
>>>    {
>>> @@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>>>            return -EINVAL;
>>>        }
>>>        if (s->header.features&   ~QED_FEATURE_MASK) {
>>> -        return -ENOTSUP; /* image uses unsupported feature bits */
>>> +        /* image uses unsupported feature bits */
>>> +        char version[64];
>>> +        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
>>>
>>>        
>> It would be useful to do s->header.features&  QED_FEATURE_MASK here as a
>> management tool doesn't know what features this version of QED supports.
>>      
> Makes sense. I'll change this for v2.
>
> Kevin
>    

[Qemu-devel] Re: [PATCH 3/4] qed: Report error for unsupported features

[Kevin Wolf]

Am 09.02.2011 12:21, schrieb Anthony Liguori:
> On 02/09/2011 04:32 AM, Kevin Wolf wrote:
>> @@ -311,7 +312,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>>           return -EINVAL;
>>       }
>>       if (s->header.features&  ~QED_FEATURE_MASK) {
>> -        return -ENOTSUP; /* image uses unsupported feature bits */
>> +        /* image uses unsupported feature bits */
>> +        char version[64];
>> +        snprintf(version, sizeof(version), "%" PRIx64, s->header.features);
>>    
> 
> It would be useful to do s->header.features & QED_FEATURE_MASK here as a 
> management tool doesn't know what features this version of QED supports.

I think you mean s->header.features & ~QED_FEATURE_MASK (small, but
important difference :-))

Kevin