It's better to return status together with setting errp. It makes
possible to avoid error propagation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h        |  2 +-
 block/qcow2-bitmap.c | 13 ++++++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index e7e662533b..49824be5c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                 Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
                                           bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f58923fce3..5eeff1cb1c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1524,9 +1524,10 @@ out:
  * readonly to begin with, and whether we opened directly or reopened to that
  * state shouldn't matter for the state we get afterward.
  */
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
                                           bool release_stored, Error **errp)
 {
+    ERRP_GUARD();
     BdrvDirtyBitmap *bitmap;
     BDRVQcow2State *s = bs->opaque;
     uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
         bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                    s->bitmap_directory_size, errp);
         if (bm_list == NULL) {
-            return;
+            return false;
         }
     }
 
@@ -1661,7 +1662,7 @@ success:
     }
 
     bitmap_list_free(bm_list);
-    return;
+    return true;
 
 fail:
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1679,16 +1680,14 @@ fail:
     }
 
     bitmap_list_free(bm_list);
+    return false;
 }
 
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
-    Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
+    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
         return -EINVAL;
     }
 
-- 
2.21.3

s/startus/status

On Wed,  9 Sep 2020 21:59:27 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h        |  2 +-
>  block/qcow2-bitmap.c | 13 ++++++-------
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index e7e662533b..49824be5c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>                                  Qcow2BitmapInfoList **info_list, Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>                                            bool release_stored, Error **errp);
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>  bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index f58923fce3..5eeff1cb1c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1524,9 +1524,10 @@ out:
>   * readonly to begin with, and whether we opened directly or reopened to that
>   * state shouldn't matter for the state we get afterward.
>   */
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>                                            bool release_stored, Error **errp)
>  {
> +    ERRP_GUARD();

Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
an error_prepend(errp, ...) not visible in the patch context.

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

>      BdrvDirtyBitmap *bitmap;
>      BDRVQcow2State *s = bs->opaque;
>      uint32_t new_nb_bitmaps = s->nb_bitmaps;
> @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>          bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                     s->bitmap_directory_size, errp);
>          if (bm_list == NULL) {
> -            return;
> +            return false;
>          }
>      }
>  
> @@ -1661,7 +1662,7 @@ success:
>      }
>  
>      bitmap_list_free(bm_list);
> -    return;
> +    return true;
>  
>  fail:
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1679,16 +1680,14 @@ fail:
>      }
>  
>      bitmap_list_free(bm_list);
> +    return false;
>  }
>  
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>  {
>      BdrvDirtyBitmap *bitmap;
> -    Error *local_err = NULL;
>  
> -    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> +    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
>          return -EINVAL;
>      }
>  

11.09.2020 12:38, Greg Kurz wrote:
> s/startus/status
> 
> On Wed,  9 Sep 2020 21:59:27 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> It's better to return status together with setting errp. It makes
>> possible to avoid error propagation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.h        |  2 +-
>>   block/qcow2-bitmap.c | 13 ++++++-------
>>   2 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index e7e662533b..49824be5c6 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>                                   Qcow2BitmapInfoList **info_list, Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>                                             bool release_stored, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>   bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index f58923fce3..5eeff1cb1c 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1524,9 +1524,10 @@ out:
>>    * readonly to begin with, and whether we opened directly or reopened to that
>>    * state shouldn't matter for the state we get afterward.
>>    */
>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>                                             bool release_stored, Error **errp)
>>   {
>> +    ERRP_GUARD();
> 
> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> an error_prepend(errp, ...) not visible in the patch context.

Ah yes. Actually this is occasional thing I didn't want to include into this patch
(and int this part I). So we can just drop it and leave for part II or part III,
or add a note into commit message

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks a lot for reviewing :)

Hmm.. With this series I understand the following:

1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:

   - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
   - reviewing is the hardest part of the process

So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.

2. So, the process turns into following steps:

   - apply scripts/coccinelle/errp-guard.cocci
   - look through patches and do obvious refactorings (like this series)
   - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)

3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)

The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).

So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..

> 
>>       BdrvDirtyBitmap *bitmap;
>>       BDRVQcow2State *s = bs->opaque;
>>       uint32_t new_nb_bitmaps = s->nb_bitmaps;
>> @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>                                      s->bitmap_directory_size, errp);
>>           if (bm_list == NULL) {
>> -            return;
>> +            return false;
>>           }
>>       }
>>   
>> @@ -1661,7 +1662,7 @@ success:
>>       }
>>   
>>       bitmap_list_free(bm_list);
>> -    return;
>> +    return true;
>>   
>>   fail:
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1679,16 +1680,14 @@ fail:
>>       }
>>   
>>       bitmap_list_free(bm_list);
>> +    return false;
>>   }
>>   
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>   {
>>       BdrvDirtyBitmap *bitmap;
>> -    Error *local_err = NULL;
>>   
>> -    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>> -    if (local_err != NULL) {
>> -        error_propagate(errp, local_err);
>> +    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
>>           return -EINVAL;
>>       }
>>   
> 


-- 
Best regards,
Vladimir

On Fri, 11 Sep 2020 13:18:32 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 11.09.2020 12:38, Greg Kurz wrote:
> > s/startus/status
> > 
> > On Wed,  9 Sep 2020 21:59:27 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> > 
> >> It's better to return status together with setting errp. It makes
> >> possible to avoid error propagation.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>   block/qcow2.h        |  2 +-
> >>   block/qcow2-bitmap.c | 13 ++++++-------
> >>   2 files changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/block/qcow2.h b/block/qcow2.h
> >> index e7e662533b..49824be5c6 100644
> >> --- a/block/qcow2.h
> >> +++ b/block/qcow2.h
> >> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> >>                                   Qcow2BitmapInfoList **info_list, Error **errp);
> >>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> >>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
> >> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >>                                             bool release_stored, Error **errp);
> >>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> >>   bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> >> index f58923fce3..5eeff1cb1c 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -1524,9 +1524,10 @@ out:
> >>    * readonly to begin with, and whether we opened directly or reopened to that
> >>    * state shouldn't matter for the state we get afterward.
> >>    */
> >> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >>                                             bool release_stored, Error **errp)
> >>   {
> >> +    ERRP_GUARD();
> > 
> > Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> > an error_prepend(errp, ...) not visible in the patch context.
> 
> Ah yes. Actually this is occasional thing I didn't want to include into this patch
> (and int this part I). So we can just drop it and leave for part II or part III,
> or add a note into commit message
> 
> > 
> > Anyway,
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Thanks a lot for reviewing :)
> 

Don't mention it :)

> Hmm.. With this series I understand the following:
> 
> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
> 
>    - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>    - reviewing is the hardest part of the process
> 
> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
> 
> 2. So, the process turns into following steps:
> 
>    - apply scripts/coccinelle/errp-guard.cocci
>    - look through patches and do obvious refactorings (like this series)
>    - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
> 

I've started to follow this process for the spapr code and, indeed, I
can come up with better changes by refactoring some code manually.
Some of these changes are not that obvious that they could be made
by someone who doesn't know the code, so I tend to agree with your
arguments in 1.

This is also the reason I didn't review patches 10, 13 and 14 because
they looked like I should understand the corresponding code a bit more.

> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
> 

Ha ha :D ... as a consolation prize, maybe we can reach a fair number
of r-b by reviewing each other's _simple_ cleanups ;-)

> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
> 

This sounds nice. My only concern would be to end up fixing code nobody
uses or cares for, so I guess it would be better that active maintainers
or supporters give impetus on that.

> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
> 

I don't :) but the very massive series that were posted on the topic
the last few months look like an announcement to me, at least for
active maintainers and supporters.

> > 
> >>       BdrvDirtyBitmap *bitmap;
> >>       BDRVQcow2State *s = bs->opaque;
> >>       uint32_t new_nb_bitmaps = s->nb_bitmaps;
> >> @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >>           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> >>                                      s->bitmap_directory_size, errp);
> >>           if (bm_list == NULL) {
> >> -            return;
> >> +            return false;
> >>           }
> >>       }
> >>   
> >> @@ -1661,7 +1662,7 @@ success:
> >>       }
> >>   
> >>       bitmap_list_free(bm_list);
> >> -    return;
> >> +    return true;
> >>   
> >>   fail:
> >>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> >> @@ -1679,16 +1680,14 @@ fail:
> >>       }
> >>   
> >>       bitmap_list_free(bm_list);
> >> +    return false;
> >>   }
> >>   
> >>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
> >>   {
> >>       BdrvDirtyBitmap *bitmap;
> >> -    Error *local_err = NULL;
> >>   
> >> -    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
> >> -    if (local_err != NULL) {
> >> -        error_propagate(errp, local_err);
> >> +    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
> >>           return -EINVAL;
> >>       }
> >>   
> > 
> 
> 

11.09.2020 14:21, Greg Kurz wrote:
> On Fri, 11 Sep 2020 13:18:32 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 11.09.2020 12:38, Greg Kurz wrote:
>>> s/startus/status
>>>
>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> It's better to return status together with setting errp. It makes
>>>> possible to avoid error propagation.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2.h        |  2 +-
>>>>    block/qcow2-bitmap.c | 13 ++++++-------
>>>>    2 files changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index e7e662533b..49824be5c6 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>                                    Qcow2BitmapInfoList **info_list, Error **errp);
>>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>>    int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>                                              bool release_stored, Error **errp);
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>>    bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index f58923fce3..5eeff1cb1c 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1524,9 +1524,10 @@ out:
>>>>     * readonly to begin with, and whether we opened directly or reopened to that
>>>>     * state shouldn't matter for the state we get afterward.
>>>>     */
>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>                                              bool release_stored, Error **errp)
>>>>    {
>>>> +    ERRP_GUARD();
>>>
>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>> an error_prepend(errp, ...) not visible in the patch context.
>>
>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>> (and int this part I). So we can just drop it and leave for part II or part III,
>> or add a note into commit message
>>
>>>
>>> Anyway,
>>>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> Thanks a lot for reviewing :)
>>
> 
> Don't mention it :)
> 
>> Hmm.. With this series I understand the following:
>>
>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>
>>     - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>     - reviewing is the hardest part of the process
>>
>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> 2. So, the process turns into following steps:
>>
>>     - apply scripts/coccinelle/errp-guard.cocci
>>     - look through patches and do obvious refactorings (like this series)
>>     - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>
> 
> I've started to follow this process for the spapr code and, indeed, I
> can come up with better changes by refactoring some code manually.
> Some of these changes are not that obvious that they could be made
> by someone who doesn't know the code, so I tend to agree with your
> arguments in 1.
> 
> This is also the reason I didn't review patches 10, 13 and 14 because
> they looked like I should understand the corresponding code a bit more.
> 
>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
>>
> 
> Ha ha :D ... as a consolation prize, maybe we can reach a fair number
> of r-b by reviewing each other's _simple_ cleanups ;-)
> 
>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
>>
> 
> This sounds nice. My only concern would be to end up fixing code nobody
> uses or cares for, so I guess it would be better that active maintainers
> or supporters give impetus on that.
> 
>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
> 
> I don't :) but the very massive series that were posted on the topic
> the last few months look like an announcement to me, at least for
> active maintainers and supporters.

Aha, I know. Better than announcement is improving checkpatch.

> 
>>>
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        BDRVQcow2State *s = bs->opaque;
>>>>        uint32_t new_nb_bitmaps = s->nb_bitmaps;
>>>> @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>            bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>>>                                       s->bitmap_directory_size, errp);
>>>>            if (bm_list == NULL) {
>>>> -            return;
>>>> +            return false;
>>>>            }
>>>>        }
>>>>    
>>>> @@ -1661,7 +1662,7 @@ success:
>>>>        }
>>>>    
>>>>        bitmap_list_free(bm_list);
>>>> -    return;
>>>> +    return true;
>>>>    
>>>>    fail:
>>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> @@ -1679,16 +1680,14 @@ fail:
>>>>        }
>>>>    
>>>>        bitmap_list_free(bm_list);
>>>> +    return false;
>>>>    }
>>>>    
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>>>    {
>>>>        BdrvDirtyBitmap *bitmap;
>>>> -    Error *local_err = NULL;
>>>>    
>>>> -    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>>>> -    if (local_err != NULL) {
>>>> -        error_propagate(errp, local_err);
>>>> +    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
>>>>            return -EINVAL;
>>>>        }
>>>>    
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 11.09.2020 14:21, Greg Kurz wrote:
>> On Fri, 11 Sep 2020 13:18:32 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 
>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>> s/startus/status
>>>>
>>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>>> It's better to return status together with setting errp. It makes
>>>>> possible to avoid error propagation.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    block/qcow2.h        |  2 +-
>>>>>    block/qcow2-bitmap.c | 13 ++++++-------
>>>>>    2 files changed, 7 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>>> index e7e662533b..49824be5c6 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>>                                    Qcow2BitmapInfoList **info_list, Error **errp);
>>>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>>>    int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>                                              bool release_stored, Error **errp);
>>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>>>    bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>>> index f58923fce3..5eeff1cb1c 100644
>>>>> --- a/block/qcow2-bitmap.c
>>>>> +++ b/block/qcow2-bitmap.c
>>>>> @@ -1524,9 +1524,10 @@ out:
>>>>>     * readonly to begin with, and whether we opened directly or reopened to that
>>>>>     * state shouldn't matter for the state we get afterward.
>>>>>     */
>>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>                                              bool release_stored, Error **errp)
>>>>>    {
>>>>> +    ERRP_GUARD();
>>>>
>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>
>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>> or add a note into commit message

Either works for me.

>>>>
>>>> Anyway,
>>>>
>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>
>>> Thanks a lot for reviewing :)
>>>
>> Don't mention it :)
>> 
>>> Hmm.. With this series I understand the following:
>>>
>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>
>>>     - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>     - reviewing is the hardest part of the process
>>>
>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.

Yes, going the extra mile is better.

I recommend it for code that is actively maintained.  Making the code
simpler and thus easier to maintain is an investment that'll pay off.

We may have code where it won't pay off.  Do you think a blind
application of errp-guard.cocci might be better than nothing there?

>>> 2. So, the process turns into following steps:
>>>
>>>     - apply scripts/coccinelle/errp-guard.cocci
>>>     - look through patches and do obvious refactorings (like this series)
>>>     - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>
>> I've started to follow this process for the spapr code and, indeed,
>> I
>> can come up with better changes by refactoring some code manually.
>> Some of these changes are not that obvious that they could be made
>> by someone who doesn't know the code, so I tend to agree with your
>> arguments in 1.
>> This is also the reason I didn't review patches 10, 13 and 14
>> because
>> they looked like I should understand the corresponding code a bit more.
>> 
>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)

LOL

A lesser craftsman than you would've peddled the generated commits
anyway.  Props!

>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>> number
>> of r-b by reviewing each other's _simple_ cleanups ;-)
>> 
>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).

If people care enough about [2] to submit patches, I'll feel obliged to
help merging them.

>> This sounds nice. My only concern would be to end up fixing code
>> nobody
>> uses or cares for, so I guess it would be better that active maintainers
>> or supporters give impetus on that.

A bit of weeding on the side is always appreciated, but please don't
feel obliged to sink lots of time into code you don't care about.

>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>
>> I don't :) but the very massive series that were posted on the topic
>> the last few months look like an announcement to me, at least for
>> active maintainers and supporters.
>
> Aha, I know. Better than announcement is improving checkpatch.

Yes, automated feedback works best.

Relentless pressure from reviewers can also work in the long, long run.
But it's tiresome.

Of course, checkpatch.pl checks only new or changed code.  Any ideas on
how to make people aware of the opportunity to simplify their existing
code?  Obvious: posting more patches simplifying existing code we care
about.  Any other smart ideas?

11.09.2020 18:22, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 11.09.2020 14:21, Greg Kurz wrote:
>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>>> s/startus/status
>>>>>
>>>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>
>>>>>> It's better to return status together with setting errp. It makes
>>>>>> possible to avoid error propagation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     block/qcow2.h        |  2 +-
>>>>>>     block/qcow2-bitmap.c | 13 ++++++-------
>>>>>>     2 files changed, 7 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>>>> index e7e662533b..49824be5c6 100644
>>>>>> --- a/block/qcow2.h
>>>>>> +++ b/block/qcow2.h
>>>>>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>>>                                     Qcow2BitmapInfoList **info_list, Error **errp);
>>>>>>     int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>>>>     int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>>                                               bool release_stored, Error **errp);
>>>>>>     int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>>>>     bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>>>> index f58923fce3..5eeff1cb1c 100644
>>>>>> --- a/block/qcow2-bitmap.c
>>>>>> +++ b/block/qcow2-bitmap.c
>>>>>> @@ -1524,9 +1524,10 @@ out:
>>>>>>      * readonly to begin with, and whether we opened directly or reopened to that
>>>>>>      * state shouldn't matter for the state we get afterward.
>>>>>>      */
>>>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>>>>                                               bool release_stored, Error **errp)
>>>>>>     {
>>>>>> +    ERRP_GUARD();
>>>>>
>>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>>
>>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>>> or add a note into commit message
> 
> Either works for me.
> 
>>>>>
>>>>> Anyway,
>>>>>
>>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>>
>>>> Thanks a lot for reviewing :)
>>>>
>>> Don't mention it :)
>>>
>>>> Hmm.. With this series I understand the following:
>>>>
>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>
>>>>      - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>>      - reviewing is the hardest part of the process
>>>>
>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
> 
> Yes, going the extra mile is better.
> 
> I recommend it for code that is actively maintained.  Making the code
> simpler and thus easier to maintain is an investment that'll pay off.
> 
> We may have code where it won't pay off.  Do you think a blind
> application of errp-guard.cocci might be better than nothing there?

I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
So, let's start from popular subsystems. And make a decision for the rest later.

> 
>>>> 2. So, the process turns into following steps:
>>>>
>>>>      - apply scripts/coccinelle/errp-guard.cocci
>>>>      - look through patches and do obvious refactorings (like this series)
>>>>      - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>>
>>> I've started to follow this process for the spapr code and, indeed,
>>> I
>>> can come up with better changes by refactoring some code manually.
>>> Some of these changes are not that obvious that they could be made
>>> by someone who doesn't know the code, so I tend to agree with your
>>> arguments in 1.
>>> This is also the reason I didn't review patches 10, 13 and 14
>>> because
>>> they looked like I should understand the corresponding code a bit more.
>>>
>>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
> 
> LOL
> 
> A lesser craftsman than you would've peddled the generated commits
> anyway.  Props!
> 
>>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>>> number
>>> of r-b by reviewing each other's _simple_ cleanups ;-)
>>>
>>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
> 
> If people care enough about [2] to submit patches, I'll feel obliged to
> help merging them.
> 
>>> This sounds nice. My only concern would be to end up fixing code
>>> nobody
>>> uses or cares for, so I guess it would be better that active maintainers
>>> or supporters give impetus on that.
> 
> A bit of weeding on the side is always appreciated, but please don't
> feel obliged to sink lots of time into code you don't care about.
> 
>>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
>>> I don't :) but the very massive series that were posted on the topic
>>> the last few months look like an announcement to me, at least for
>>> active maintainers and supporters.
>>
>> Aha, I know. Better than announcement is improving checkpatch.
> 
> Yes, automated feedback works best.
> 
> Relentless pressure from reviewers can also work in the long, long run.
> But it's tiresome.
> 
> Of course, checkpatch.pl checks only new or changed code.  Any ideas on
> how to make people aware of the opportunity to simplify their existing
> code?  Obvious: posting more patches simplifying existing code we care
> about.  Any other smart ideas?
> 

I think it would be great for checkpatch to warn about some code problems in function touched by checked patch. Code-style, error-api, etc. This will attract developers to improve existing code they are interested in.


-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 11.09.2020 18:22, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 11.09.2020 14:21, Greg Kurz wrote:
>>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
[...]
>>>>> Hmm.. With this series I understand the following:
>>>>>
>>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>>
>>>>>      - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>>>      - reviewing is the hardest part of the process
>>>>>
>>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> Yes, going the extra mile is better.
>>
>> I recommend it for code that is actively maintained.  Making the code
>> simpler and thus easier to maintain is an investment that'll pay off.
>>
>> We may have code where it won't pay off.  Do you think a blind
>> application of errp-guard.cocci might be better than nothing there?
>
> I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
> So, let's start from popular subsystems. And make a decision for the rest later.

Makes sense.

[...]

On Wed 09 Sep 2020 08:59:27 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  [...]
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>                                            bool release_stored, Error **errp)
>  {
> +    ERRP_GUARD();

This needs an explanation.

Berto