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
