1. Patchew
  2. QEMU
  3. [PATCH v6 00/10] parallels: Refactor the code of images checks and fix a bug
  4. View patch

[PATCH v6 07/10] parallels: Move check of cluster outside image to a separate function

Alexander Ivanov posted 10 patches 3 years, 5 months ago
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, "Denis V. Lunev" <den@openvz.org>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
There is a newer version of this series
[PATCH v6 07/10] parallels: Move check of cluster outside image to a separate function
Posted by Alexander Ivanov 3 years, 5 months ago 
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.

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

diff --git a/block/parallels.c b/block/parallels.c
index eea318f809..f50cd232aa 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -438,13 +438,50 @@ static void parallels_check_unclean(BlockDriverState *bs,
     }
 }
 
+static int parallels_check_outside_image(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+    int64_t off, high_off, size;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
+
+    high_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off > size) {
+            fprintf(stderr, "%s cluster %u is outside image\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            res->corruptions++;
+            if (fix & BDRV_FIX_ERRORS) {
+                parallels_set_bat_entry(s, i, 0);
+                res->corruptions_fixed++;
+            }
+            continue;
+        }
+        if (high_off < off) {
+            high_off = off;
+        }
+    }
+
+    s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS;
+
+    return 0;
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t size, prev_off, high_off;
-    int ret = 0;
+    int ret;
     uint32_t i;
 
     size = bdrv_getlength(bs->file->bs);
@@ -457,6 +494,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
     parallels_check_unclean(bs, res, fix);
 
+    ret = parallels_check_outside_image(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
@@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             continue;
         }
 
-        /* cluster outside the image */
-        if (off > size) {
-            fprintf(stderr, "%s cluster %u is outside image\n",
-                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
-            res->corruptions++;
-            if (fix & BDRV_FIX_ERRORS) {
-                parallels_set_bat_entry(s, i, 0);
-                res->corruptions_fixed++;
-            }
-            prev_off = 0;
-            continue;
-        }
-
         res->bfi.allocated_clusters++;
         if (off > high_off) {
             high_off = off;
@@ -519,8 +548,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         }
     }
 
-    s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
 out:
     qemu_co_mutex_unlock(&s->lock);
 
-- 
2.34.1
Re: [PATCH v6 07/10] parallels: Move check of cluster outside image to a separate function
Posted by Denis V. Lunev 3 years, 5 months ago 
On 25.08.2022 16:31, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 59 ++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index eea318f809..f50cd232aa 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -438,13 +438,50 @@ static void parallels_check_unclean(BlockDriverState *bs,
>       }
>   }
>   
> +static int parallels_check_outside_image(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t i;
> +    int64_t off, high_off, size;
> +
> +    size = bdrv_getlength(bs->file->bs);
> +    if (size < 0) {
> +        res->check_errors++;
> +        return size;
> +    }
> +
> +    high_off = 0;
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > size) {

we need one more patch here. Correct check would be

if (off >= size) {
   bla-bla()
}

> +            fprintf(stderr, "%s cluster %u is outside image\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +            res->corruptions++;
> +            if (fix & BDRV_FIX_ERRORS) {
> +                parallels_set_bat_entry(s, i, 0);
> +                res->corruptions_fixed++;
> +            }
> +            continue;
> +        }
> +        if (high_off < off) {
> +            high_off = off;
> +        }
> +    }
> +
> +    s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS;
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
>                                              BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
>       int64_t size, prev_off, high_off;
> -    int ret = 0;
> +    int ret;
>       uint32_t i;
>   
>       size = bdrv_getlength(bs->file->bs);
> @@ -457,6 +494,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>       parallels_check_unclean(bs, res, fix);
>   
> +    ret = parallels_check_outside_image(bs, res, fix);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>       res->bfi.total_clusters = s->bat_size;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
> @@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               continue;
>           }
>   
> -        /* cluster outside the image */
> -        if (off > size) {
> -            fprintf(stderr, "%s cluster %u is outside image\n",
> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> -            res->corruptions++;
> -            if (fix & BDRV_FIX_ERRORS) {
> -                parallels_set_bat_entry(s, i, 0);
> -                res->corruptions_fixed++;
> -            }
> -            prev_off = 0;
> -            continue;
> -        }
> -
>           res->bfi.allocated_clusters++;
>           if (off > high_off) {
>               high_off = off;
> @@ -519,8 +548,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>           }
>       }
>   
> -    s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
> -
>   out:
>       qemu_co_mutex_unlock(&s->lock);
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>
Re: [PATCH v6 07/10] parallels: Move check of cluster outside image to a separate function
Posted by Alexander Ivanov 3 years, 5 months ago 
On 26.08.2022 15:08, Denis V. Lunev wrote:
> On 25.08.2022 16:31, Alexander Ivanov wrote:
>> We will add more and more checks so we need a better code structure
>> in parallels_co_check. Let each check performs in a separate loop
>> in a separate helper.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 59 ++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 43 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index eea318f809..f50cd232aa 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -438,13 +438,50 @@ static void 
>> parallels_check_unclean(BlockDriverState *bs,
>>       }
>>   }
>>   +static int parallels_check_outside_image(BlockDriverState *bs,
>> +                                         BdrvCheckResult *res,
>> +                                         BdrvCheckMode fix)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    uint32_t i;
>> +    int64_t off, high_off, size;
>> +
>> +    size = bdrv_getlength(bs->file->bs);
>> +    if (size < 0) {
>> +        res->check_errors++;
>> +        return size;
>> +    }
>> +
>> +    high_off = 0;
>> +    for (i = 0; i < s->bat_size; i++) {
>> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> +        if (off > size) {
>
> we need one more patch here. Correct check would be
>
> if (off >= size) {
>   bla-bla()
> }
I have such a patch in the next patchset.
>
>> +            fprintf(stderr, "%s cluster %u is outside image\n",
>> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>> +            res->corruptions++;
>> +            if (fix & BDRV_FIX_ERRORS) {
>> +                parallels_set_bat_entry(s, i, 0);
>> +                res->corruptions_fixed++;
>> +            }
>> +            continue;
>> +        }
>> +        if (high_off < off) {
>> +            high_off = off;
>> +        }
>> +    }
>> +
>> +    s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS;
>> +
>> +    return 0;
>> +}
>> +
>>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>>                                              BdrvCheckResult *res,
>>                                              BdrvCheckMode fix)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>>       int64_t size, prev_off, high_off;
>> -    int ret = 0;
>> +    int ret;
>>       uint32_t i;
>>         size = bdrv_getlength(bs->file->bs);
>> @@ -457,6 +494,11 @@ static int coroutine_fn 
>> parallels_co_check(BlockDriverState *bs,
>>         parallels_check_unclean(bs, res, fix);
>>   +    ret = parallels_check_outside_image(bs, res, fix);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>>       res->bfi.total_clusters = s->bat_size;
>>       res->bfi.compressed_clusters = 0; /* compression is not 
>> supported */
>>   @@ -469,19 +511,6 @@ static int coroutine_fn 
>> parallels_co_check(BlockDriverState *bs,
>>               continue;
>>           }
>>   -        /* cluster outside the image */
>> -        if (off > size) {
>> -            fprintf(stderr, "%s cluster %u is outside image\n",
>> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>> -            res->corruptions++;
>> -            if (fix & BDRV_FIX_ERRORS) {
>> -                parallels_set_bat_entry(s, i, 0);
>> -                res->corruptions_fixed++;
>> -            }
>> -            prev_off = 0;
>> -            continue;
>> -        }
>> -
>>           res->bfi.allocated_clusters++;
>>           if (off > high_off) {
>>               high_off = off;
>> @@ -519,8 +548,6 @@ static int coroutine_fn 
>> parallels_co_check(BlockDriverState *bs,
>>           }
>>       }
>>   -    s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
>> -
>>   out:
>>       qemu_co_mutex_unlock(&s->lock);
> Reviewed-by: Denis V. Lunev <den@openvz.org>
Re: [PATCH v6 07/10] parallels: Move check of cluster outside image to a separate function
Posted by Denis V. Lunev 3 years, 5 months ago 
On 26.08.2022 15:23, Alexander Ivanov wrote:
>
> On 26.08.2022 15:08, Denis V. Lunev wrote:
>> On 25.08.2022 16:31, Alexander Ivanov wrote:
>>> We will add more and more checks so we need a better code structure
>>> in parallels_co_check. Let each check performs in a separate loop
>>> in a separate helper.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>>   block/parallels.c | 59 
>>> ++++++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 43 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index eea318f809..f50cd232aa 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -438,13 +438,50 @@ static void 
>>> parallels_check_unclean(BlockDriverState *bs,
>>>       }
>>>   }
>>>   +static int parallels_check_outside_image(BlockDriverState *bs,
>>> +                                         BdrvCheckResult *res,
>>> +                                         BdrvCheckMode fix)
>>> +{
>>> +    BDRVParallelsState *s = bs->opaque;
>>> +    uint32_t i;
>>> +    int64_t off, high_off, size;
>>> +
>>> +    size = bdrv_getlength(bs->file->bs);
>>> +    if (size < 0) {
>>> +        res->check_errors++;
>>> +        return size;
>>> +    }
>>> +
>>> +    high_off = 0;
>>> +    for (i = 0; i < s->bat_size; i++) {
>>> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>>> +        if (off > size) {
>>
>> we need one more patch here. Correct check would be
>>
>> if (off >= size) {
>>   bla-bla()
>> }
> I have such a patch in the next patchset.

If you have it in hands, can you pls send it here as 11/10
patch. That would be most convenient to me.

Den

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.