On 7/4/19 11:06 AM, Max Reitz wrote:
> 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?
>
Ehm, it just honestly didn't occur to me that I could acquire the
context before doing the input sanitizing.
In this case, it is OK to do it, so I will hoist it back up into
do_blockdev_backup.
--js
>> 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?
>
Inertia.
> Other than that:
>
> Reviewed-by: Max Reitz <mre...@redhat.com>
>
Suggestions applied, thank you.