17.10.2019 15:04, Peter Maydell wrote: > On Thu, 10 Oct 2019 at 12:44, Max Reitz <mre...@redhat.com> wrote: >> >> From: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> >> 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