Re: [PATCH PoC 1/7] QAPI: gpio JSON

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Mar 19, 2025 at 10:57:51AM +0300, Nikita Shubin via B4 Relay wrote:
>> From: Nikita Shubin <nshubin@yadro.com>
>> 
>> Signed-off-by: Nikita Shubin <nshubin@yadro.com>

The commit message should briefly explain the purpose of the change.

>> ---
>>  qapi/gpio.json        | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/meson.build      |  1 +
>>  qapi/qapi-schema.json |  1 +
>>  3 files changed, 70 insertions(+)
>> 
>> diff --git a/qapi/gpio.json b/qapi/gpio.json
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..1c2b7af36813ff52cbb3a44e64a2e5a5d8658d62
>> --- /dev/null
>> +++ b/qapi/gpio.json
>> @@ -0,0 +1,68 @@
>> +# -*- Mode: Python -*-
>> +# vim: filetype=python
>> +#
>> +
>> +##
>> +# = Gpio devices

Spell it GPIO in doc text, because it's an acronym.

>> +##
>> +
>> +##
>> +# @GpiodevInfo:
>> +#
>> +# Information about a gpio device.
>> +#
>> +# @label: the label of the gpio device
>> +#
>> +# Since: 9.2

Make that 10.1 everywhere.

>> +##
>> +{ 'struct': 'GpiodevInfo',
>> +  'data': { 'label': 'str' } }
>> +
>> +##
>> +# @GpiodevBackendKind:
>> +#
>> +# @chardev: chardevs
>> +#
>> +# Since: 9.2
>> +##
>> +{ 'enum': 'GpiodevBackendKind',
>> +  'data': [ 'chardev' ] }
>> +
>> +##
>> +# @GpiodevChardev:
>> +#
>> +# Configuration info for chardev gpiodevs.
>> +#
>> +# @chardev: chardev id
>> +#
>> +# @size: buffer size, default is 65536

What buffer is being sized here?

>> +#
>> +# Since: 9.2
>> +##
>> +  { 'struct': 'GpiodevChardev',
>> +    'data': { 'chardev': 'str',
>> +              '*size': 'int' } }

Use 'size' instead of 'int' for byte counts, please.

>> +
>> +##
>> +# @GpiodevChardevWrapper:
>> +#
>> +# @data: Configuration info for chardev gpiodevs
>> +#
>> +# Since: 9.2
>> +##
>> +{ 'struct': 'GpiodevChardevWrapper',
>> +  'data': { 'data': 'GpiodevChardev' } }
>> +
>> +##
>> +# @GpiodevBackend:
>> +#
>> +# Configuration info for the new chardev backend.
>> +#
>> +# @type: backend type
>> +#
>> +# Since: 9.2
>> +##
>> +{ 'union': 'GpiodevBackend',
>> +  'base': { 'type': 'GpiodevBackendKind' },
>> +  'discriminator': 'type',
>> +  'data': { 'chardev': 'GpiodevChardevWrapper' } }

Why GpiodevChardevWrapper?  Consistency with similar existing things?

The wrapper types are remnants of "simple" unions.  New code should
avoid them, unless avoiding them would make things hard to use.  For
instance, when adding a new branch to a union where the existing
branches are of such wrapper types, use a wrapper for the new one to
keep things consistent.

What other backend types do you have in mind?

> While historically we've just wired things up to chardevs in QEMU,
> in most cases this is just a hack to get the ability too configure
> a socket, with the other chardev backends being never used. The
> downside of this is that chardev APIs internally are not very nice
> to work with, especally if you want/need to be aware of client
> connection establishment/closure.
>
> These days we've got common socket APIs and QAPI schema available
> and can bypass the chardevs (which have a pretty unpleasant
> internal API) if all we need is a socket backend connecting to
> an external server. This would let code directly work with the
> QIOChannelSocket object, instead of that object being hidden
> behind the chardev APIs.

Good point.  Nikita, do you see a use for chardevs other than sockets?

[...]