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
