diff mbox series

[06/21] parallels: refactor path when we need to re-check image in parallels_open

Message ID 20230915184130.403366-8-den@openvz.org
State New
Headers show
Series implement discard operation for Parallels images | expand

Commit Message

Denis V. Lunev Sept. 15, 2023, 6:41 p.m. UTC 
More conditions follows thus the check should be more scalable.

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

Comments

Mike Maslenkin Sept. 17, 2023, 11:40 a.m. UTC | #1 
This patch generates a warning.

On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev <den@openvz.org> wrote:
>
> More conditions follows thus the check should be more scalable.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  block/parallels.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 8f223bfd89..aa29df9f77 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>      int ret, size, i;
>      int64_t file_nb_sectors, sector;
>      uint32_t data_start;
> -    bool data_off_is_correct;
> +    bool need_check = false;
>
>      ret = parallels_opts_prealloc(bs, options, errp);
>      if (ret < 0) {
> @@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>      s->bat_bitmap = (uint32_t *)(s->header + 1);
>
>      if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        s->header_unclean = true;
> +        need_check = s->header_unclean = true;
>      }
>
> -    data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
> -                                                  &data_start);
> +    need_check = need_check ||
> +                 !parallels_test_data_off(s, file_nb_sectors, &data_start);
> +

../block/parallels.c:1139:18: warning: variable 'data_start' is used
uninitialized whenever '||' condition is true
[-Wsometimes-uninitialized]
    need_check = need_check ||
                 ^~~~~~~~~~
../block/parallels.c:1142:21: note: uninitialized use occurs here
    s->data_start = data_start;
                    ^~~~~~~~~~
../block/parallels.c:1139:18: note: remove the '||' if its condition
is always false
    need_check = need_check ||
                 ^~~~~~~~~~~~~
../block/parallels.c:1067:24: note: initialize the variable
'data_start' to silence this warning
    uint32_t data_start;
                       ^
                        = 0
1 warning generated.


Regards,
Mike.
Denis V. Lunev Sept. 17, 2023, 1:04 p.m. UTC | #2 
On 9/17/23 13:40, Mike Maslenkin wrote:
> This patch generates a warning.
>
> On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev <den@openvz.org> wrote:
>> More conditions follows thus the check should be more scalable.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 19 ++++++++-----------
>>   1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 8f223bfd89..aa29df9f77 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>       int ret, size, i;
>>       int64_t file_nb_sectors, sector;
>>       uint32_t data_start;
>> -    bool data_off_is_correct;
>> +    bool need_check = false;
>>
>>       ret = parallels_opts_prealloc(bs, options, errp);
>>       if (ret < 0) {
>> @@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>>
>>       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>> -        s->header_unclean = true;
>> +        need_check = s->header_unclean = true;
>>       }
>>
>> -    data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
>> -                                                  &data_start);
>> +    need_check = need_check ||
>> +                 !parallels_test_data_off(s, file_nb_sectors, &data_start);
>> +
> ../block/parallels.c:1139:18: warning: variable 'data_start' is used
> uninitialized whenever '||' condition is true
> [-Wsometimes-uninitialized]
>      need_check = need_check ||
>                   ^~~~~~~~~~
> ../block/parallels.c:1142:21: note: uninitialized use occurs here
>      s->data_start = data_start;
>                      ^~~~~~~~~~
> ../block/parallels.c:1139:18: note: remove the '||' if its condition
> is always false
>      need_check = need_check ||
>                   ^~~~~~~~~~~~~
> ../block/parallels.c:1067:24: note: initialize the variable
> 'data_start' to silence this warning
>      uint32_t data_start;
>                         ^
>                          = 0
> 1 warning generated.
>
>
> Regards,
> Mike.
wow! That is pretty much correct!

What compiler/OS you are using :)

Den
Alexander Ivanov Sept. 18, 2023, 8:31 a.m. UTC | #3 
On 9/15/23 20:41, Denis V. Lunev wrote:
> More conditions follows thus the check should be more scalable.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 8f223bfd89..aa29df9f77 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       int ret, size, i;
>       int64_t file_nb_sectors, sector;
>       uint32_t data_start;
> -    bool data_off_is_correct;
> +    bool need_check = false;
>   
>       ret = parallels_opts_prealloc(bs, options, errp);
>       if (ret < 0) {
> @@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
>       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        s->header_unclean = true;
> +        need_check = s->header_unclean = true;
>       }
>   
> -    data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
> -                                                  &data_start);
> +    need_check = need_check ||
> +                 !parallels_test_data_off(s, file_nb_sectors, &data_start);
> +
>       s->data_start = data_start;
>       s->data_end = s->data_start;
>       if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
> @@ -1194,6 +1195,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>               s->data_end = sector + s->tracks;
>           }
>       }
> +    need_check = need_check || s->data_end > file_nb_sectors;
>   
>       /*
>        * We don't repair the image here if it's opened for checks. Also we don't
> @@ -1203,12 +1205,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           return 0;
>       }
>   
> -    /*
> -     * Repair the image if it's dirty or
> -     * out-of-image corruption was detected.
> -     */
> -    if (s->data_end > file_nb_sectors || s->header_unclean
> -        || !data_off_is_correct) {
> +    /* Repair the image if corruption was detected. */
> +    if (need_check) {
>           BdrvCheckResult res;
>           ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>           if (ret < 0) {
> @@ -1217,7 +1215,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>               goto fail;
>           }
>       }
> -
>       return 0;
>   
>   fail_format:

Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
diff mbox series

Patch

diff --git a/block/parallels.c b/block/parallels.c
index 8f223bfd89..aa29df9f77 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1065,7 +1065,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     int ret, size, i;
     int64_t file_nb_sectors, sector;
     uint32_t data_start;
-    bool data_off_is_correct;
+    bool need_check = false;
 
     ret = parallels_opts_prealloc(bs, options, errp);
     if (ret < 0) {
@@ -1133,11 +1133,12 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
     if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-        s->header_unclean = true;
+        need_check = s->header_unclean = true;
     }
 
-    data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
-                                                  &data_start);
+    need_check = need_check ||
+                 !parallels_test_data_off(s, file_nb_sectors, &data_start);
+
     s->data_start = data_start;
     s->data_end = s->data_start;
     if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1194,6 +1195,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
             s->data_end = sector + s->tracks;
         }
     }
+    need_check = need_check || s->data_end > file_nb_sectors;
 
     /*
      * We don't repair the image here if it's opened for checks. Also we don't
@@ -1203,12 +1205,8 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         return 0;
     }
 
-    /*
-     * Repair the image if it's dirty or
-     * out-of-image corruption was detected.
-     */
-    if (s->data_end > file_nb_sectors || s->header_unclean
-        || !data_off_is_correct) {
+    /* Repair the image if corruption was detected. */
+    if (need_check) {
         BdrvCheckResult res;
         ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
         if (ret < 0) {
@@ -1217,7 +1215,6 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
     }
-
     return 0;
 
 fail_format: