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.sist...@oracle.com> > > Acked-by: Peter Xu <pet...@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? > >> */ >
