Re: [RFC PATCH] qom/object: Disallow comma in type names

Re: [RFC PATCH] qom/object: Disallow comma in type names

[Thomas Huth]

Forgot to CC: qemu-devel (sorry) - thanks to Markus for the hint.
So let's repeat it here:

On 14/11/2023 10.31, Daniel P. Berrangé wrote:
> On Tue, Nov 14, 2023 at 10:05:37AM +0100, Thomas Huth wrote:
>> QOM names currently don't have any enforced naming rules. This can
>> be problematic, e.g. when they are used on the command line for
>> the "-device" option (where the comma is used to separate properties).
>> To avoid that such problematic type names come in again, let's
>> disallow them now by adding an g_assert() during the type registration.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   Based-on: <20231113134344.1195478-1-armbru@redhat.com>
>>   (without Markus' patches, the g_assert() triggers with the current
>>    code base)
>>   
>>   See discussion here:
>>   https://lore.kernel.org/qemu-devel/87y1f0hjdh.fsf@pond.sub.org/
>>
>>   Questions: Should we disallow other characters, too? Slash and
>>   backslash maybe (since they can cause trouble with module names)?
>>   Dot and colon would maybe be good candidates, too, but they seem
>>   to be in wide use already, so these don't really seem to be
>>   feasible...
> 
> There's two questions.
> 
>    * What should we enforce today
>    * What should we ideally enforce in future
> 
> Ideally the answers would be the same, but getting there will
> almost certainly require some cleanup first.
> 
> Given that we can now define QOM types using QAPI, I feel we
> preserve everyone's sanity by enforcing the same rules for
> QOM and QAPI type naming. IOW
> 
>    All QOM type names must begin with a letter, and contain
>    only ASCII letters, digits, hyphen, and underscore.
> 
> is the answer for the second question.
> 
> In terms of what we can enforce today, we can block ',',
> but we can't block '.' without some cleanup, and possibly
> the same for ':'. Can we assume we don't have any other
> non-alphanumeric chars used ?
> 
> If so, I think that today we we could probably get away with
> saying:
> 
>    All QOM type names must begin with a letter, and contain
>    only ASCII letters, digits, hyphen, underscore, period
>    and colon. Usage of period and colon is deprecated.
> 
>>   qom/object.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 95c0dc8285..85461ab0d2 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -113,6 +113,8 @@ static TypeImpl *type_new(const TypeInfo *info)
>>           abort();
>>       }
>>   
>> +    g_assert(!strchr(info->name, ','));
> 
> 
> So with helpers:
> 
>    #define QOM_VALID_TYPECHARS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLNMOPQRSTUVWXYZ0123456789-_.:"
> 
>    static bool qom_valid_typename(const char *name) {
>       return name[0] != '\0' &&
>          g_ascii_isalpha(name[0]) &&
>          strspn(name, QOM_VALID_TYPECHARS) == strlen(name));
>    }
> 
> The check would then become
> 
>       g_assert(qom_valid_typename(info->name));
> 
>       if (strstr(info->name, '.') ||
>           strstr(info->name, ':')) {
>          warn_report("Usage of '.' and ':', in type name %s is deprecated", info->name)
>       }
> 
> 
> The warn_report could be a bit annoying if we the unusual type names are
> something we're going to hit frequently ?
> 
> With regards,
> Daniel

I've added a debug printf, and seems like we register a lot of types like:

  cfi.pflash01
  virt-2.6-machine::hotplug-handler
  aspeed.i2c.slave::vmstate-if
  pc-i440fx-3.0-machine::nmi

... just to name some few. So I don't think it is feasible to use the 
warn_report here.

  Thomas

Re: [RFC PATCH] qom/object: Disallow comma in type names

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Forgot to CC: qemu-devel (sorry) - thanks to Markus for the hint.
> So let's repeat it here:
>
> On 14/11/2023 10.31, Daniel P. Berrangé wrote:
>> On Tue, Nov 14, 2023 at 10:05:37AM +0100, Thomas Huth wrote:
>>> QOM names currently don't have any enforced naming rules. This can
>>> be problematic, e.g. when they are used on the command line for
>>> the "-device" option (where the comma is used to separate properties).
>>> To avoid that such problematic type names come in again, let's
>>> disallow them now by adding an g_assert() during the type registration.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Based-on: <20231113134344.1195478-1-armbru@redhat.com>
>>>   (without Markus' patches, the g_assert() triggers with the current
>>>    code base)
>>>   
>>>   See discussion here:
>>>   https://lore.kernel.org/qemu-devel/87y1f0hjdh.fsf@pond.sub.org/
>>>
>>>   Questions: Should we disallow other characters, too? Slash and
>>>   backslash maybe (since they can cause trouble with module names)?
>>>   Dot and colon would maybe be good candidates, too, but they seem
>>>   to be in wide use already, so these don't really seem to be
>>>   feasible...
>> 
>> There's two questions.
>> 
>>    * What should we enforce today
>>    * What should we ideally enforce in future
>> 
>> Ideally the answers would be the same, but getting there will
>> almost certainly require some cleanup first.
>> 
>> Given that we can now define QOM types using QAPI, I feel we
>> preserve everyone's sanity by enforcing the same rules for
>> QOM and QAPI type naming. IOW

Agree!

>>    All QOM type names must begin with a letter, and contain
>>    only ASCII letters, digits, hyphen, and underscore.
>> 
>> is the answer for the second question.

As long as type names only occur as *values*, the next sentence's first
exception applies, too:

      There are two exceptions: enum values may start with a digit, and
      names that are downstream extensions (see section `Downstream
      extensions`_) start with underscore.

This is of course docs/devel/qapi-code-gen.rst.

I'm willing to tweak the QAPI naming rules within reason.

>> In terms of what we can enforce today, we can block ',',
>> but we can't block '.' without some cleanup, and possibly
>> the same for ':'. Can we assume we don't have any other
>> non-alphanumeric chars used ?

I ran qom-list-types (without 'abstract': true) for all 31
qemu-system-FOO, extracted the type names, and sorted them into buckets.

* I found 3255 distinct names

* 2445 names conform to the QAPI naming rule "only letters, digits,
  hyphen, and underscore, starting with a letter"

* 157 more names conform with the enum exception "may start with a
  digit"

* The remainder contain unwanted characters

  - 9 contain ',' and no other unwanted characters

    My "hw: Replace anti-social QOM type names (again)" fixes them.

  - 638 contain '.' and no other unwanted characters

    That's a lot.

    Perhaps we can permit '.' in enum names.  Needs thought.

  - 6 contain '.' and '+'

    Sun-UltraSparc-IIIi+-sparc64-cpu
    Sun-UltraSparc-IV+-sparc64-cpu
    power5+_v2.1-powerpc64-cpu
    power5+_v2.1-spapr-cpu-core
    power7+_v2.1-powerpc64-cpu
    power7+_v2.1-spapr-cpu-core

    Spell out "plus"?

I found no names with ':'.  Looks like we use ':' only for abstract
types (which includes interfaces).

The only #define TYPE_FOO with a colon I can see is

    #define TYPE_RAM_DISCARD_MANAGER "qemu:ram-discard-manager"

An interface type.  Let's ditch the "qemu:".

There are a bunch of interface names containing "::".  These come from
type_initialize_interface():

    info.name = g_strdup_printf("%s::%s", ti->name, interface_type->name);

>> If so, I think that today we we could probably get away with
>> saying:
>> 
>>    All QOM type names must begin with a letter, and contain
>>    only ASCII letters, digits, hyphen, underscore, period
>>    and colon. Usage of period and colon is deprecated.

I think we should reserve colon for QOM internal use.

[...]