[v8] parallels: Refactor the code of images checks and fix a bug

[PATCH v8 11/11] parallels: Incorrect condition in out-of-image check

Posted by Alexander Ivanov 3 years ago

All the offsets in the BAT must be lower than the file size.
Fix the check condition for correct check.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 621dbf623a..eda3fb558d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -455,7 +455,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     high_off = 0;
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off > size) {
+        if (off >= size) {
             fprintf(stderr, "%s cluster %u is outside image\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
-- 
2.34.1

Re: [PATCH v8 11/11] parallels: Incorrect condition in out-of-image check

Posted by Hanna Czenczek 3 years ago

On 15.01.23 16:58, Alexander Ivanov wrote:
> All the offsets in the BAT must be lower than the file size.
> Fix the check condition for correct check.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 621dbf623a..eda3fb558d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -455,7 +455,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       high_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
>           off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> -        if (off > size) {
> +        if (off >= size) {

Should this not be the even stricter `off + s->cluster_size > size` 
instead, or is it possible to have partial clusters at the image end?

Hanna

>               fprintf(stderr, "%s cluster %u is outside image\n",
>                       fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>               res->corruptions++;

Re: [PATCH v8 11/11] parallels: Incorrect condition in out-of-image check

Posted by Alexander Ivanov 3 years ago


On 18.01.2023 15:46, Hanna Czenczek wrote:
> On 15.01.23 16:58, Alexander Ivanov wrote:
>> All the offsets in the BAT must be lower than the file size.
>> Fix the check condition for correct check.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 621dbf623a..eda3fb558d 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -455,7 +455,7 @@ static int 
>> parallels_check_outside_image(BlockDriverState *bs,
>>       high_off = 0;
>>       for (i = 0; i < s->bat_size; i++) {
>>           off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> -        if (off > size) {
>> +        if (off >= size) {
>
> Should this not be the even stricter `off + s->cluster_size > size` 
> instead, or is it possible to have partial clusters at the image end?
>
> Hanna
'off' is aligned* on the cluster size, so these conditions are 
equivalent, but I agree, your condition is more idiomatic.

* It works for the new image format. In the old one there could be 
entries in the BAT, pointing to unaligned clusters. There will be a 
separate check for unaligned clusters.
>
>>               fprintf(stderr, "%s cluster %u is outside image\n",
>>                       fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>>               res->corruptions++;
>