Repair an image at opening if the image is unclean or
out-of-image corruption was detected.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 95 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 08526196da..a7c3af4ef2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     return ret;
 }
 
+typedef struct ParallelsOpenCheckCo {
+    BlockDriverState *bs;
+    BdrvCheckResult *res;
+    BdrvCheckMode fix;
+    int ret;
+} ParallelsOpenCheckCo;
+
+static void coroutine_fn parallels_co_open_check(void *opaque)
+{
+    ParallelsOpenCheckCo *poc = opaque;
+    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
+}
 
 static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
                                             Error **errp)
@@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
-    int ret, size, i;
-    int64_t file_size;
+    int ret, size;
+    int64_t file_size, high_off;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1027,34 +1039,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
-    for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i);
-        if (off >= file_size) {
-            if (flags & BDRV_O_CHECK) {
-                continue;
-            }
-            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
-                       "is larger than file size (%" PRIi64 ")",
-                       off, i, file_size);
-            ret = -EINVAL;
-            goto fail;
-        }
-        if (off >= s->data_end) {
-            s->data_end = off + s->tracks;
-        }
-    }
-
-    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-        /* Image was not closed correctly. The check is mandatory */
-        s->header_unclean = true;
-        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
-            error_setg(errp, "parallels: Image was not closed correctly; "
-                       "cannot be opened read/write");
-            ret = -EACCES;
-            goto fail;
-        }
-    }
-
     opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
     if (!opts) {
         goto fail_options;
@@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         error_free(s->migration_blocker);
         goto fail;
     }
+
     qemu_co_mutex_init(&s->lock);
+
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        s->header_unclean = true;
+    }
+
+    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
+    if (high_off >= s->data_end) {
+        s->data_end = high_off + s->tracks;
+    }
+
+    /*
+     * We don't repair the image here if it is opened for checks.
+     * Also let to work with images in RO mode.
+     */
+    if ((flags & BDRV_O_CHECK) || !(flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+
+    /*
+     * Repair the image if it's dirty or
+     * out-of-image corruption was detected.
+     */
+    if (s->data_end > file_size ||
+        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        BdrvCheckResult res = {0};
+        Coroutine *co;
+        ParallelsOpenCheckCo poc = {
+            .bs = bs,
+            .res = &res,
+            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
+            .ret = -EINPROGRESS
+        };
+
+        if (qemu_in_coroutine()) {
+            /* From bdrv_co_create.  */
+            parallels_co_open_check(&poc);
+        } else {
+            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+            co = qemu_coroutine_create(parallels_co_open_check, &poc);
+            qemu_coroutine_enter(co);
+            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
+        }
+
+        if (poc.ret < 0) {
+            error_setg_errno(errp, -poc.ret,
+                             "Could not repair corrupted image");
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_format:
-- 
2.34.1

On 9/2/22 10:53, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or
> out-of-image corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 95 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 65 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 08526196da..a7c3af4ef2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> +typedef struct ParallelsOpenCheckCo {
> +    BlockDriverState *bs;
> +    BdrvCheckResult *res;
> +    BdrvCheckMode fix;
> +    int ret;
> +} ParallelsOpenCheckCo;
> +
> +static void coroutine_fn parallels_co_open_check(void *opaque)
> +{
> +    ParallelsOpenCheckCo *poc = opaque;
> +    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
> +}
>   
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
> @@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1027,34 +1039,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
> -        s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks.
> +     * Also let to work with images in RO mode.
> +     */
> +    if ((flags & BDRV_O_CHECK) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_size ||
> +        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        BdrvCheckResult res = {0};
> +        Coroutine *co;
> +        ParallelsOpenCheckCo poc = {
> +            .bs = bs,
> +            .res = &res,
> +            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
> +            .ret = -EINPROGRESS
> +        };
> +
> +        if (qemu_in_coroutine()) {
> +            /* From bdrv_co_create.  */
> +            parallels_co_open_check(&poc);
> +        } else {
> +            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +            co = qemu_coroutine_create(parallels_co_open_check, &poc);
> +            qemu_coroutine_enter(co);
> +            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
> +        }
> +
> +        if (poc.ret < 0) {
> +            error_setg_errno(errp, -poc.ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
bdrv_check() is your friend. No need to duplicate the code


>       return 0;
>   
>   fail_format:

On 9/2/22 10:53, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or
> out-of-image corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 95 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 65 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 08526196da..a7c3af4ef2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> +typedef struct ParallelsOpenCheckCo {
> +    BlockDriverState *bs;
> +    BdrvCheckResult *res;
> +    BdrvCheckMode fix;
> +    int ret;
> +} ParallelsOpenCheckCo;
> +
> +static void coroutine_fn parallels_co_open_check(void *opaque)
> +{
> +    ParallelsOpenCheckCo *poc = opaque;
> +    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
> +}
>   
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
> @@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1027,34 +1039,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
> -        s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks.
> +     * Also let to work with images in RO mode.
My silly $0.02.
Also let *us allow* to with in read-only more.

> +     */
> +    if ((flags & BDRV_O_CHECK) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
not enough, We are not allowed to make changes with O_INACTIVE.
The check in this case should be postponed till O_INACTIVE clearance.

Very specific note. header_unclean is allowed in O_INACTIVE.
The image could be opened at the moment on the other
host!

This should be clarified.

> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_size ||
> +        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
I dislike this. There are detection conditions above
and I think we should respect them adding the flag
'need_check' which should be kept in the BDS to
respect O_INACTIVE.

> +        BdrvCheckResult res = {0};
such assignments are weird (not portable for older compilers).

> +        Coroutine *co;
> +        ParallelsOpenCheckCo poc = {
> +            .bs = bs,
> +            .res = &res,
This is strange, why not to put BdrvCheckResult as
a whole to the ParallelsOpenCheckCo
> +            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
> +            .ret = -EINPROGRESS
pls add comma on the line above, This is common convention
as in the case of addition one more initializing field you will
not change that line.
> +        };
> +
> +        if (qemu_in_coroutine()) {
> +            /* From bdrv_co_create.  */
> +            parallels_co_open_check(&poc);
> +        } else {
> +            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +            co = qemu_coroutine_create(parallels_co_open_check, &poc);
> +            qemu_coroutine_enter(co);
> +            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
> +        }
> +
> +        if (poc.ret < 0) {
> +            error_setg_errno(errp, -poc.ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format: