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 <js...@redhat.com>
> ---
> 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 <mre...@redhat.com>
> +
> +out:
> + aio_context_release(aio_context);
> + return job;
> +}
signature.asc
Description: OpenPGP digital signature
