Re: [PATCH v3 0/3] proper locking on bitmap add/remove paths

[Vladimir Sementsov-Ogievskiy]

26.09.2019 22:01, John Snow wrote:
> 
> 
> On 9/20/19 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> We need to lock qcow2 mutex on accessing in-image metadata, especially
>> on updating this metadata. Let's implement it.
>>
>> v3:
>> 01: add John's r-b
>> 02: - fix bdrv_remove_persistent_dirty_bitmap return value
>>      - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap
>> 03: add John's r-b
>>
>> Vladimir Sementsov-Ogievskiy (3):
>>    block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
>>    block/dirty-bitmap: return int from
>>      bdrv_remove_persistent_dirty_bitmap
>>    block/qcow2: proper locking on bitmap add/remove paths
>>
>>   block/qcow2.h                |  14 ++---
>>   include/block/block_int.h    |  14 ++---
>>   include/block/dirty-bitmap.h |   5 +-
>>   block.c                      |  22 -------
>>   block/dirty-bitmap.c         | 119 +++++++++++++++++++++++++++++++++--
>>   block/qcow2-bitmap.c         |  36 +++++++----
>>   block/qcow2.c                |   5 +-
>>   blockdev.c                   |  28 +++------
>>   8 files changed, 163 insertions(+), 80 deletions(-)
>>
> 
> I'll take this; I imagine the return signatures are going to change
> again with your error propagation series, though ...?
> 

Thanks a lot!

Hmm, I don't think so, as I used to think that returning int for errp-functions
is better anyway..

ret = f(..., errp);
if (ret < 0) {

}

vs

f(..., errp);
if (*errp) {

}

Hmmm... The latter just looks unfamiliar in comparison with "if (ret < 0)".. But
if we anyway going to convert a lot of "if (*local_err)" to "if (*errp)", it 
will
become familiar.. And the latter may save 6 characters in a line with function 
call,
which may save the whole line in some places.

So I don't know.

returning two errors is not very good, we don't have convention for it actually.

if I have int ret = f(..., errp), what should I report?

error_report_err_errno(ret, errp), or just error_report_err(errp), assuming errp
contains the whole information?

Still, sometimes we need to distinguish one error code from another, and we 
can't
check errp for such thing..

-- 
Best regards,
Vladimir