[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V2] migration: simplify blockers
From: |
Steven Sistare |
Subject: |
Re: [PATCH V2] migration: simplify blockers |
Date: |
Tue, 7 Feb 2023 16:17:34 -0500 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 |
On 2/7/2023 3:05 PM, Peter Xu wrote:
> On Tue, Feb 07, 2023 at 11:03:13AM -0800, Steve Sistare wrote:
>> Modify migrate_add_blocker and migrate_del_blocker to take an Error **
>> reason. This allows migration to own the Error object, so that if
>> an error occurs, migration code can free the Error and clear the client
>> handle, simplifying client code. This is a pre-requisite for a future
>> patch that will allow one Error blocker to be registered for multiple
>> migration modes.
>>
>> No functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>
> Acked-by: Peter Xu <peterx@redhat.com>
>
> One trivial comment below.
>
> [...]
>
>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>> index 9cebe2b..7c8d326 100644
>> --- a/include/migration/blocker.h
>> +++ b/include/migration/blocker.h
>> @@ -17,19 +17,22 @@
>> /**
>> * @migrate_add_blocker - prevent migration from proceeding
>> *
>> - * @reason - an error to be returned whenever migration is attempted
>> + * @reasonp - address of an error to be returned whenever migration is
>> attempted
>> *
>> * @errp - [out] The reason (if any) we cannot block migration right now.
>> *
>> * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
>> + *
>> + * *@reasonp is freed and set to NULL if failure is returned.
>> + * On success, the caller no longer owns *@reasonp and must not free it.
>
> This statement reads weird. IMHO the caller still owns @reasonp, but if it
> succeeded it should only free it with a migrate_del_blocker() later.
How about:
On success, the caller must not free @reasonp, except by calling
migrate_del_blocker.
Same below.
- Steve
>> */
>> -int migrate_add_blocker(Error *reason, Error **errp);
>> +int migrate_add_blocker(Error **reasonp, Error **errp);
>>
>> /**
>> * @migrate_add_blocker_internal - prevent migration from proceeding without
>> * only-migrate implications
>> *
>> - * @reason - an error to be returned whenever migration is attempted
>> + * @reasonp - address of an error to be returned whenever migration is
>> attempted
>> *
>> * @errp - [out] The reason (if any) we cannot block migration right now.
>> *
>> @@ -38,14 +41,19 @@ int migrate_add_blocker(Error *reason, Error **errp);
>> * Some of the migration blockers can be temporary (e.g., for a few
>> seconds),
>> * so it shouldn't need to conflict with "-only-migratable". For those
>> cases,
>> * we can call this function rather than @migrate_add_blocker().
>> + *
>> + * *@reasonp is freed and set to NULL if failure is returned.
>> + * On success, the caller no longer owns *@reasonp and must not free it.
>
> Same here?
>
>> */
>