Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:

Now we support extensions saving and can let to work with them in
read-write mode.

Signed-off-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>
---
  block/parallels-ext.c |  4 ----
  block/parallels.c     | 17 ++++-------------
  2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, 
size_t data_size,
          return NULL;
      }

- /* We support format extension only for RO parallels images. */

-    assert(!(bs->open_flags & BDRV_O_RDWR));
-    bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
      return bitmap;
  }

diff --git a/block/parallels.c b/block/parallels.c

index a49922c6a7..d5d87984cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
      }

if (ph.ext_off) {

-        if (flags & BDRV_O_RDWR) {
-            /*
-             * It's unsafe to open image RW if there is an extension (as we
-             * don't support it). But parallels driver in QEMU historically
-             * ignores the extension, so print warning and don't care.
-             */
-            warn_report("Format Extension ignored in RW mode");
-        } else {
-            ret = parallels_read_format_extension(
-                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-            if (ret < 0) {
-                goto fail;
-            }
+        ret = parallels_read_format_extension(
+                bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+        if (ret < 0) {
+            goto fail;
          }
      }

something like attached should be taken into the account.
Though the destiny of cluster with old
extension offset requires some thinking.

I would say that it could be marked as used on read.
Anyway, this requires at least detailed thinking.
From 2f70166ef640304726d5dfcee3e906b0ba1676dd Mon Sep 17 00:00:00 2001
From: "Denis V. Lunev" <d...@openvz.org>
Date: Thu, 18 Jan 2024 13:29:56 +0100
Subject: [PATCH 1/1] parallels: drop dirty bitmap data if the image was not
 properly closed

This data is obsolete.

The approach is exactly the same like we use with QCOW2.

Signed-off-by: Denis V. Lunev <d...@openvz.org>
---
 block/parallels-ext.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..54e8bb66a6 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,14 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
             return 0;
 
         case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+            if (s->header_unclean) {
+                /*
+                 * The image was not closed correctly and thus dirty bitmap
+                 * data inside the image is considered as incorrect and thus
+                 * it should be dropper, exactly like we do for QCOW2.
+                 */
+                break;
+            }
             bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
             if (!bitmap) {
                 goto fail;
-- 
2.34.1