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

[Peter Maydell]

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):

> @@ -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