[Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Jeff Cody]

If there is a dirty log file to be replayed in a VHDX image, it is
replayed in .vhdx_open().  However, if the file is opened read-only,
then a somewhat cryptic error message results.

This adds a more helpful error message for the user.  If an image file
contains a log to be replayed, and is opened read-only, the user is
instructed to run 'qemu-img check -r all' on the image file.

Running qemu-img check -r all will cause the image file to be opened
r/w, which will replay the log file.  If a log file replay is detected,
this is flagged, and bdrv_check will increase the corruptions_fixed
count for the image.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 12 +++++++++++-
 block/vhdx.c     | 22 ++++++++++++++++++++--
 block/vhdx.h     |  5 ++++-
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index ee5583c..34c563e 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -706,7 +706,8 @@ exit:
  *
  * If read-only, we must replay the log in RAM (or refuse to open
  * a dirty VHDX file read-only) */
-int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed)
+int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
+                   Error **errp)
 {
     int ret = 0;
     VHDXHeader *hdr;
@@ -761,6 +762,15 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed)
     }
 
     if (logs.valid) {
+        if (bs->read_only) {
+            ret = -EPERM;
+            error_setg_errno(errp, EPERM,
+                             "VHDX image file '%s' opened read-only, but "
+                             "contains a log that needs replayed.  To replay "
+                             "the log, execute:\n qemu-img check -r all '%s'",
+                             bs->filename, bs->filename);
+            goto exit;
+        }
         /* now flush the log */
         ret = vhdx_log_flush(bs, s, &logs);
         if (ret < 0) {
diff --git a/block/vhdx.c b/block/vhdx.c
index 67bbe10..1995778 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -878,7 +878,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     int ret = 0;
     uint32_t i;
     uint64_t signature;
-    bool log_flushed = false;
 
 
     s->bat = NULL;
@@ -907,7 +906,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = vhdx_parse_log(bs, s, &log_flushed);
+    ret = vhdx_parse_log(bs, s, &s->log_replayed_on_open, errp);
     if (ret < 0) {
         goto fail;
     }
@@ -1854,6 +1853,24 @@ exit:
     return ret;
 }
 
+/* If opened r/w, the VHDX driver will automatically replay the log,
+ * if one is present, inside the vhdx_open() call.
+ *
+ * If qemu-img check -r all is called, the image is automatically opened
+ * r/w and any log has already been replayed, so there is nothing (currently)
+ * for us to do here
+ */
+static int vhdx_check(BlockDriverState *bs, BdrvCheckResult *result,
+                       BdrvCheckMode fix)
+{
+    BDRVVHDXState *s = bs->opaque;
+
+    if (s->log_replayed_on_open) {
+        result->corruptions_fixed++;
+    }
+    return 0;
+}
+
 static QEMUOptionParameter vhdx_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1898,6 +1915,7 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_co_writev         = vhdx_co_writev,
     .bdrv_create            = vhdx_create,
     .bdrv_get_info          = vhdx_get_info,
+    .bdrv_check             = vhdx_check,
 
     .create_options         = vhdx_create_options,
 };
diff --git a/block/vhdx.h b/block/vhdx.h
index 51183b2..2acd7c2 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -394,6 +394,8 @@ typedef struct BDRVVHDXState {
 
     Error *migration_blocker;
 
+    bool log_replayed_on_open;
+
     QLIST_HEAD(VHDXRegionHead, VHDXRegionEntry) regions;
 } BDRVVHDXState;
 
@@ -408,7 +410,8 @@ uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
 
 bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, int crc_offset);
 
-int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed);
+int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
+                   Error **errp);
 
 int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s,
                              void *data, uint32_t length, uint64_t offset);
-- 
1.8.3.1

Re: [Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On 12/17/2013 03:33 AM, Jeff Cody wrote:
> If there is a dirty log file to be replayed in a VHDX image, it is
> replayed in .vhdx_open().  However, if the file is opened read-only,
> then a somewhat cryptic error message results.
> 
> This adds a more helpful error message for the user.  If an image file
> contains a log to be replayed, and is opened read-only, the user is
> instructed to run 'qemu-img check -r all' on the image file.
> 
> Running qemu-img check -r all will cause the image file to be opened
> r/w, which will replay the log file.  If a log file replay is detected,
> this is flagged, and bdrv_check will increase the corruptions_fixed
> count for the image.

> +            error_setg_errno(errp, EPERM,
> +                             "VHDX image file '%s' opened read-only, but "
> +                             "contains a log that needs replayed.  To replay "

s/replayed/replaying/

> +                             "the log, execute:\n qemu-img check -r all '%s'",
> +                             bs->filename, bs->filename);
> +            goto exit;

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Jeff Cody]

On Tue, Dec 17, 2013 at 06:56:41AM -0700, Eric Blake wrote:
> On 12/17/2013 03:33 AM, Jeff Cody wrote:
> > If there is a dirty log file to be replayed in a VHDX image, it is
> > replayed in .vhdx_open().  However, if the file is opened read-only,
> > then a somewhat cryptic error message results.
> > 
> > This adds a more helpful error message for the user.  If an image file
> > contains a log to be replayed, and is opened read-only, the user is
> > instructed to run 'qemu-img check -r all' on the image file.
> > 
> > Running qemu-img check -r all will cause the image file to be opened
> > r/w, which will replay the log file.  If a log file replay is detected,
> > this is flagged, and bdrv_check will increase the corruptions_fixed
> > count for the image.
> 
> > +            error_setg_errno(errp, EPERM,
> > +                             "VHDX image file '%s' opened read-only, but "
> > +                             "contains a log that needs replayed.  To replay "
> 
> s/replayed/replaying/
> 

How about "to be replayed" instead?  "Replaying", while correct, just
sounds a bit off to me.

> > +                             "the log, execute:\n qemu-img check -r all '%s'",
> > +                             bs->filename, bs->filename);
> > +            goto exit;
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On 12/17/2013 08:38 AM, Jeff Cody wrote:
>>
>>> +            error_setg_errno(errp, EPERM,
>>> +                             "VHDX image file '%s' opened read-only, but "
>>> +                             "contains a log that needs replayed.  To replay "
>>
>> s/replayed/replaying/
>>
> 
> How about "to be replayed" instead?  "Replaying", while correct, just
> sounds a bit off to me.

Yes, that works as well.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [qemu-kvm PATCH] block: vhdx - improve error message, and .bdrv_check implementation

[Stefan Hajnoczi]

On Tue, Dec 17, 2013 at 05:33:37AM -0500, Jeff Cody wrote:
> If there is a dirty log file to be replayed in a VHDX image, it is
> replayed in .vhdx_open().  However, if the file is opened read-only,
> then a somewhat cryptic error message results.
> 
> This adds a more helpful error message for the user.  If an image file
> contains a log to be replayed, and is opened read-only, the user is
> instructed to run 'qemu-img check -r all' on the image file.
> 
> Running qemu-img check -r all will cause the image file to be opened
> r/w, which will replay the log file.  If a log file replay is detected,
> this is flagged, and bdrv_check will increase the corruptions_fixed
> count for the image.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 12 +++++++++++-
>  block/vhdx.c     | 22 ++++++++++++++++++++--
>  block/vhdx.h     |  5 ++++-
>  3 files changed, 35 insertions(+), 4 deletions(-)

Merged "to be replayed" typo fix when applying the patch.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan