Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in block/replication.c

[Alberto Garcia]

On Mon 08 Oct 2018 04:34:24 AM CEST, Max Reitz wrote:
>> This patch does the following changes:
>> 
>>   - Use an options QDict with the "read-only" option instead of
>>     passing the changes as flags only.
>> 
>>   - Simplify the code (it was unnecessarily complicated and verbose).
>> 
>>   - Fix a bug due to which the secondary disk was not being put back
>>     in read-only mode when writable=false (because in this case
>>     orig_secondary_flags always had the BDRV_O_RDWR flag set).
>> 
>>   - Stop clearing the BDRV_O_INACTIVE flag.
>
> Why?  Sorry, but I don't know much about replication.  Do you know
> this change is OK?  (Since the code deliberately clears it right now.)

I don't think the current code is ok, the BDRV_O_INACTIVE flag should be
cleared with bdrv_co_invalidate_cache() so I'm inclined to think that
it's a bug.

>> @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, 
>> bool writable,
>>  {
>>      BDRVReplicationState *s = bs->opaque;
>>      BlockReopenQueue *reopen_queue = NULL;
>> -    int orig_hidden_flags, orig_secondary_flags;
>> -    int new_hidden_flags, new_secondary_flags;
>>      Error *local_err = NULL;
>>  
>>      if (writable) {
>
> I'd like to request a comment that "writable" is true the first time
> this function is called, and false the second time.  That'd make it
> clear why this rewrite makes sense.

Yes, I think it's probably a good idea to do that.

>> +        s->orig_hidden_read_only =
>> +            !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR);
>> +        s->orig_secondary_read_only =
>> +            !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR);
>
> Why not use bdrv_is_read_only()?

No reason, I overlooked this. I'll fix it.

Berto