[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers
From: |
Peter Maydell |
Subject: |
Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers |
Date: |
Thu, 17 Oct 2019 13:04:42 +0100 |
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
- [PULL 26/36] iotests: Fix 125 for growth_mode = metadata, (continued)
- [PULL 26/36] iotests: Fix 125 for growth_mode = metadata, Max Reitz, 2019/10/10
- [PULL 28/36] iotests: Use stat -c %b in 125, Max Reitz, 2019/10/10
- [PULL 29/36] block/backup: move in-flight requests handling from backup to block-copy, Max Reitz, 2019/10/10
- [PULL 30/36] block/backup: move write_flags calculation inside backup_job_create, Max Reitz, 2019/10/10
- [PULL 31/36] block/block-copy: split block_copy_set_callbacks function, Max Reitz, 2019/10/10
- [PULL 34/36] nbd: add empty .bdrv_reopen_prepare, Max Reitz, 2019/10/10
- [PULL 32/36] block: introduce backup-top filter driver, Max Reitz, 2019/10/10
- [PULL 36/36] iotests/162: Fix for newer Linux 5.3+, Max Reitz, 2019/10/10
- [PULL 35/36] tests: fix I/O test for hosts defaulting to LUKSv2, Max Reitz, 2019/10/10
- [PULL 33/36] block/backup: use backup-top instead of write notifiers, Max Reitz, 2019/10/10
- Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers,
Peter Maydell <=
- Re: [PULL 00/36] Block patches, Peter Maydell, 2019/10/14