Re: [PATCH v3 3/4] migration/qapi: Replace @MigrateSetParameters with @MigrationParameters

[Peter Xu <peterx@redhat.com>]

On Mon, Oct 09, 2023 at 02:25:10PM +0200, Markus Armbruster wrote:
> I apologize for the late reply.

Not a problem - one week is not even bad at all. :)

[...]

> > One thing I can do is to move qdev_prop_str_or_null impl (from you) into a
> > separate patch.   I can drop some unnecessary changes too when possible,
> > not yet sure what else I can split, but I can try once there.
> 
> Suggest to give it a quick try, then see whether you like the resulting
> split better than what you have now.

OK.

[...]

> >> > diff --git a/migration/options.c b/migration/options.c
> >> > index 6bbfd4853d..12e392f68c 100644
> >> > --- a/migration/options.c
> >> > +++ b/migration/options.c
> >> > @@ -164,9 +164,12 @@ Property migration_properties[] = {
> >> >      DEFINE_PROP_SIZE("announce-step", MigrationState,
> >> >                        parameters.announce_step,
> >> >                        DEFAULT_MIGRATE_ANNOUNCE_STEP),
> >> > -    DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
> >> > -    DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
> >> > -    DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
> >> > +    DEFINE_PROP_STR_OR_NULL("tls-creds", MigrationState,
> >> > +                            parameters.tls_creds),
> >> > +    DEFINE_PROP_STR_OR_NULL("tls-hostname", MigrationState,
> >> > +                            parameters.tls_hostname),
> >> > +    DEFINE_PROP_STR_OR_NULL("tls-authz", MigrationState,
> >> > +                            parameters.tls_authz),
> >> 
> >> The qdev machinery now additionally accepts JSON null as property
> >> value.
> >> 
> >> If that's undesirable, we need to reject it.
> >
> > I don't think we have a need to pass in null here, not to mention this is
> > only for debugging purpose.
> 
> Not sure I understand the bit about debugging.

The migration_properties here is only used by "-global migration.XXX=YYY".
It's not expected for a normal user to use this interface; one should
normally use QMP or even HMP.  So migration_properties as a whole is for
debugging purpose.

> 
> The point I was trying to make is this.  Before the patch, we reject
> attempts to set the property value to null.  Afterwards, we accept them,
> i.e. the patch loses "reject null property value".  If this loss is
> undesirable, we better replace it with suitable hand-written code.

I don't even know how to set it to NULL before.. as it can only be accessed
via cmdline "-global" as mentioned above, which must be a string anyway.
So I assume this is not an issue.

> 
> > The real problem here, IMHO, is current patch will crash if someone
> > specifies -global migration.tls_* fields..
> 
> Trips this assertion:
> 
>     bool visit_start_alternate(Visitor *v, const char *name,
>                                GenericAlternate **obj, size_t size,
>                                Error **errp)
>     {
>         bool ok;
> 
>         assert(obj && size >= sizeof(GenericAlternate));
>         assert(!(v->type & VISITOR_OUTPUT) || *obj);
>         trace_visit_start_alternate(v, name, obj, size);
>         if (!v->start_alternate) {
> --->        assert(!(v->type & VISITOR_INPUT));
>             return true;
>         }
>         ok = v->start_alternate(v, name, obj, size, errp);
>         if (v->type & VISITOR_INPUT) {
>             assert(ok != !*obj);
>         }
>         return ok;
>     }
> 
> Documented with the start_alternate() method in visitor-impl.h:
> 
>         /* Must be set by input and clone visitors to visit alternates */
>         bool (*start_alternate)(Visitor *v, const char *name,
>                                 GenericAlternate **obj, size_t size,
>                                 Error **errp);
> 
> Known restriction of the string input visitor:
> 
>     /*
>      * The string input visitor does not implement support for visiting
>      * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
>      * of integers (except type "size") are supported.
>      */
>     Visitor *string_input_visitor_new(const char *str);
> 
> A similar restriction is documented for the string output visitor.
> 
> > Unfortunately I'm not an expert on qapi.  Markus, does something like this
> > look like a valid fix to you?
> 
> I'm not sure whether your simple patch is sufficient for lifting the
> restriction.  Needs a deeper think, I'm afraid.  Can we make progress on
> the remainder of the series in parallel?

We can move back to using string rather than StrOrNull, but I saw there's
another email discussing this.  Let me also read that first, before jumping
back and forth on the solutions.

Note that my goal prior to this series is introducing another migration
parameter (avail-switchover-bandwidth).  What I think I can do is I'll
proceed with that patch rebasing to master rather than this series; doing
the triplication once more and decouple.  Then we can postpone this series.

Thanks,

-- 
Peter Xu