Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

17.10.2019 15:04, Peter Maydell wrote:
> On Thu, 10 Oct 2019 at 12:44, Max Reitz <address@hidden> wrote:
>>
>> From: Vladimir Sementsov-Ogievskiy <address@hidden>
>>
>> Drop write notifiers and use filter node instead.
> 
> Hi; after this change Coverity complains about dead code in
> backup_job_create() (CID 1406402):

Oops, I'm sorry. Will send a patch soon.

> 
>> @@ -382,6 +353,8 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>       BackupBlockJob *job = NULL;
>>       int64_t cluster_size;
>>       BdrvRequestFlags write_flags;
>> +    BlockDriverState *backup_top = NULL;
>> +    BlockCopyState *bcs = NULL;
>>
>>       assert(bs);
>>       assert(target);
>> @@ -463,33 +436,35 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>       write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING 
>> : 0) |
>>                     (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
>>
>> +    backup_top = bdrv_backup_top_append(bs, target, filter_node_name,
>> +                                        cluster_size, write_flags, &bcs, 
>> errp);
>> +    if (!backup_top) {
>> +        goto error;
>> +    }
>> +
>>       /* job->len is fixed, so we can't allow resize */
>> -    job = block_job_create(job_id, &backup_job_driver, txn, bs, 0, 
>> BLK_PERM_ALL,
>> +    job = block_job_create(job_id, &backup_job_driver, txn, backup_top,
>> +                           0, BLK_PERM_ALL,
>>                              speed, creation_flags, cb, opaque, errp);
>>       if (!job) {
>>           goto error;
>>       }
>>
>> +    job->backup_top = backup_top;
>>       job->source_bs = bs;
>>       job->on_source_error = on_source_error;
>>       job->on_target_error = on_target_error;
>>       job->sync_mode = sync_mode;
>>       job->sync_bitmap = sync_bitmap;
>>       job->bitmap_mode = bitmap_mode;
>> -
>> -    job->bcs = block_copy_state_new(bs, target, cluster_size, write_flags,
>> -                                    errp);
>> -    if (!job->bcs) {
>> -        goto error;
>> -    }
>> -
> 
> This code deletion has removed the only code path that can
> reach the 'error' label after the successful creation of the job...
> 
>> +    job->bcs = bcs;
>>       job->cluster_size = cluster_size;
>>       job->len = len;
>>
>> -    block_copy_set_callbacks(job->bcs, backup_progress_bytes_callback,
>> +    block_copy_set_callbacks(bcs, backup_progress_bytes_callback,
>>                                backup_progress_reset_callback, job);
>>
>> -    /* Required permissions are already taken by block-copy-state target */
>> +    /* Required permissions are already taken by backup-top target */
>>       block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>                          &error_abort);
>>
>> @@ -502,6 +477,8 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>       if (job) {
>>           backup_clean(&job->common.job);
>>           job_early_fail(&job->common.job);
> 
> ...so down here in the 'error:' code the "if (job)" condition
> can never pass, and the code in this part of the if statement
> is dead.
> 
>> +    } else if (backup_top) {
>> +        bdrv_backup_top_drop(backup_top);
>>       }
>>
>>       return NULL;
>>   {"return": {}}
> 
> thanks
> -- PMM
> 


-- 
Best regards,
Vladimir