Re: [Qemu-devel] [PATCH v2 02/18] drive-backup: create do_backup_common

[Max Reitz]

On 03.07.19 23:55, John Snow wrote:
> Create a common core that comprises the actual meat of what the backup API
> boundary needs to do, and then switch drive-backup to use it.
> 
> Questions:
>  - do_drive_backup now acquires and releases the aio_context in addition
>    to do_backup_common doing the same. Can I drop this from drive_backup?

I wonder why you don’t just make it a requirement that
do_backup_common() is called with the context acquired?

> Signed-off-by: John Snow <address@hidden>
> ---
>  blockdev.c | 138 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 83 insertions(+), 55 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4d141e9a1f..5fd663a7e5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3425,6 +3425,86 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +static BlockJob *do_backup_common(BackupCommon *backup,
> +                                  BlockDriverState *target_bs,
> +                                  JobTxn *txn, Error **errp)
> +{

[...]

> +    job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
> +                            backup->sync, bmap, backup->compress,
> +                            backup->on_source_error, backup->on_target_error,
> +                            job_flags, NULL, NULL, txn, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        goto out;
> +    }

Below, you change do_drive_backup() to just pass errp instead of
local_err and not do error handling.  Why not do the same here?

Other than that:

Reviewed-by: Max Reitz <address@hidden>

> +
> +out:
> +    aio_context_release(aio_context);
> +    return job;
> +}

signature.asc
Description: OpenPGP digital signature