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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> 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. :)

I try to keep conversations moving once they started.

> [...]
>
>> > 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.

Something like

    {"execute": "migrate-set-parameters",
     "arguments": {"tls-authz": null}}

Hmm, crashes in migrate_params_apply(), which is a bug.  I'm getting
more and more suspicious about user-facing migration code...

If the migration object is accessible with qom-set, then that's another
way to assign null values.

>> > 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.

In my "QAPI string visitors crashes" memo, I demonstrated that the crash
on funny property type predates your series.  You merely add another
instance.  Moreover, crashing -global is less serious than a crashing
monitor command, because only the latter can take down a running guest.
Can't see why your series needs to wait for a fix of the crash bug.
Makes sense?

> 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,