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(-)
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(-)
--
2.21.0
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(-) > Thanks, applied to my bitmaps tree: https://github.com/jnsnow/qemu/commits/bitmaps https://github.com/jnsnow/qemu.git --js
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 ...?
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
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
© 2016 - 2024 Red Hat, Inc.