Re: [PATCH] migration: Don't free the reason after calling migrate_add_blocker

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Oct 24, 2025 at 12:17:20PM -0400, Peter Xu wrote:
> On Fri, Oct 24, 2025 at 04:08:39PM +0200, Markus Armbruster wrote:
> > Markus Armbruster <armbru@redhat.com> writes:
> > 
> > > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > [...]
> > 
> > >> But wow, the migrate_add_blocker API design is unpleasant with its
> > >> pair of "Error **" parameters - it is practically designed to
> > >> maximise confusion & surprise.
> > >
> > > It's quite a sight, isn't it?
> > >
> > > I'll give it a quick Friday afternoon try.
> > 
> > Alright, my confusion has been maximised.  Giving up on this.
> 
> Besides the use of two Error** that might be confusing, what is more
> confusing (if not wrong..): migrate_add_blocker() will take ownership of
> the 1st Error**, no matter whether the helper succeeded or not. However, it
> only resets the first Error** if failed.
> 
> I think it means if migrate_add_blocker() succeeded, the caller will have a
> non-NULL pointer, even if it has lost the ownership of that pointer.
> 
> I'm guessing it never caused issue only because we don't usually
> error_free() the migration blocker anywhere.. but I think maybe we should
> at least do an error_copy() in add_blockers()..

IMHO we should not even be using an Error object for the the blocker.
AFAICT, internally all we care about is the formatted string. The main
reason for using an Error object appears to be to have a convenient
pointer to use as an identifier to later pass to del_blocker.

I'd be inclined to just have passed in a fixed string, and return an
integer identifier for the blocker. eg

    int64 migrate_add_blocker(const char *reason, Error **errp);

    void migrate_del_blocker(int64 blockerid);

The migrate_add_blocker method would strdup(reason) to keep its own
copy.

The usage would thus be clear & simple:

    int64 blockerid = migrate_add_blocker("cannot migrate vfio", errp);
    if (!blockerid) {
         return;
    }

    ... some time later...

    migrate_del_blocker(blockerid);


In some cases we needed dynamically formatted strings, which could have
been achieved thus:

    g_autofree char *msg = g_strdup_printf("cannot migrate vfio %d", blah);
    int64 blockerid = migrate_add_blocker(msg, errp);
    ...the rest as above...

yes, this costs an extra strdup(), but that is an acceptable & negligible
overhead in the context in which we're doing this.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|