Re: [PATCH v2] block/backup-top: fix flags handling

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

06.02.2020 19:31, Max Reitz wrote:
> On 03.02.20 14:42, Vladimir Sementsov-Ogievskiy wrote:
>> backup-top "supports" write-unchanged, by skipping CBW operation in
>> backup_top_co_pwritev. But it forgets to do the same in
>> backup_top_co_pwrite_zeroes, as well as declare support for
>> BDRV_REQ_WRITE_UNCHANGED.
>>
>> Fix this, and, while being here, declare also support for flags
>> supported by source child.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> v2: restrict flags propagation like it is done in other filters [Eric]
>>      move state variable initialization to the top
>>
>>   block/backup-top.c | 32 ++++++++++++++++++++------------
>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/backup-top.c b/block/backup-top.c
>> index 9aed2eb4c0..a4cec60859 100644
>> --- a/block/backup-top.c
>> +++ b/block/backup-top.c
> 
> 
> [...]
> 
>> @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>                                            Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    BDRVBackupTopState *state;
>>       BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>>                                                    filter_node_name,
>>                                                    BDRV_O_RDWR, errp);
>> +    BDRVBackupTopState *state = top->opaque;
>>   
>>       if (!top) {
>>           return NULL;
>>       }
> 
> If top can be NULL, then we shouldn’t dereference it before (with
> state = top->opaque).

Oh yes.

> 
> (Pulling up the initialization of @state is also unrelated to this
> patch, I don’t exactly know why you’re doing it here.)
> 

I just though, that it is small good style enhancement, but it isn't :)

Let's keep state variable handling as it was.

-- 
Best regards,
Vladimir