Re: [Qemu-devel] [PATCH 4/8] block/backup: hoist bitmap check into QMP interface

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1643 bytes --]

On 10.07.19 19:57, John Snow wrote:
> 
> 
> On 7/10/19 12:11 PM, Max Reitz wrote:
>> On 10.07.19 03:05, John Snow wrote:
>>> This is nicer to do in the unified QMP interface that we have now,
>>> because it lets us use the right terminology back at the user.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  block/backup.c | 13 ++++---------
>>>  blockdev.c     | 10 ++++++++++
>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index e2729cf6fa..a64b768e24 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -566,6 +566,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>>      assert(bs);
>>>      assert(target);
>>>  
>>> +    /* QMP interface protects us from these cases */
>>> +    assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
>>> +    assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
>>
>> Implication would be a nice operator sometimes.
>>
>> ("assert(sync_mode == MIRROR_SYNC_MODE_BITMAP -> sync_bitmap)")
>>
>> (Can you do that in C++?  No, you can’t overload bool’s operators, right?)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
> 
> Yes, I also find this assertion kind of hard to read personally, but it
> feels somewhat clunky to write:
> 
> if (antecedent) {
>     assert(condition);
> }
> 
> I suppose we can also phrase this as:
> 
> assert(sync_mode == MIRROR_SYNC_MODE_BITMAP ? sync_bitmap : true);
> 
> Which might honestly be pretty good. Mind if I change it to this?

Looks weird (mostly unfamiliar), but I do not.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]