[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/3] parallels: Put the image checks in separate functions

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 1/3] parallels: Put the image checks in separate functions
Date: Sat, 6 Aug 2022 22:00:20 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

A bit strange to see this. Is your name set up correctly in ~/.gitconfig ?

We will add more and more checks of images so we need to reorganize the code.
Put each check to a separate helper function with a separate loop.
Add two helpers: truncate_file() and sync_header(). They will be used
in multiple functions.

It would be a lot simpler to review if you split out one check_ function per 
patch :)

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>


-            /*
-             * In order to really repair the image, we must shrink it.
-             * That means we have to pass exact=true.
-             */
-            ret = bdrv_truncate(bs->file, res->image_end_offset, true,
-                                PREALLOC_MODE_OFF, 0, &local_err);
+            ret = truncate_file(bs, res->image_end_offset);
              if (ret < 0) {
-                error_report_err(local_err);
-                res->check_errors++;

Why you remove check_errors++ ? It's an error of truncate_file counted.
With it kept here:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-                goto out;
+                return ret;
              res->leaks_fixed += count;


+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+    qemu_co_mutex_lock(&s->lock);

If touch this anyway, I'd move to QEMU_LOCK_GUARD().

+    check_unclean(bs, res, fix);
+    ret = check_cluster_outside_image(bs, res, fix);
+    if (ret != 0) {
+        goto out;
+    }
+    check_fragmentation(bs, res, fix);
+    ret = check_leak(bs, res, fix);
+    if (ret != 0) {
+        goto out;
+    }
+    collect_statistics(bs, res);

probably, chack_fragmentation() should be part of collect_statistics() (called 
from it, or just merged into same loop, as it fills same res->bfi)

Also while reviewing I noted that when FIX_ERRORS is not enabled we count broken 
clusters (with off > size) as allocated, and also they participate in 
fragmentation statistics. That's preexisting still..

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]