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