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

[John Snow]

On 9/27/19 4:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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..
> 

OK, well, no problem. I'm not very picky about the error propagation
paradigm; since you are investing your effort in it lately I'm just
going to trust your judgment here.

> 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..
> 

OK, I just wasn't sure the details of your series, actually -- I just
wanted to know if we'd need to make changes, but if not, that's easier :)

--js