Re: [RFC 01/15] scripts/qapi: support type-based unions

2024-03-28 Thread Vladimir Sementsov-Ogievskiy

On 28.03.24 12:15, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Look at block-job-change command: we have to specify both 'id' to chose
the job to operate on and 'type' for QAPI union be parsed. But for user
this looks redundant: when we specify 'id', QEMU should be able to get
corresponding job's type.

This commit brings such a possibility: just specify some Enum type as
'discriminator', and define a function somewhere with prototype

bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)

Further commits will use this functionality to upgrade block-job-change
interface and introduce some new interfaces.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/qapi/introspect.py |  5 +++-
  scripts/qapi/schema.py | 50 +++---
  scripts/qapi/types.py  |  3 ++-
  scripts/qapi/visit.py  | 43 ++--
  4 files changed, 73 insertions(+), 28 deletions(-)


I believe you need to update docs/devel/qapi-code-gen.rst.

Current text:

 Union types
 ---

 Syntax::

 UNION = { 'union': STRING,
   'base': ( MEMBERS | STRING ),
   'discriminator': STRING,
   'data': BRANCHES,
   '*if': COND,
   '*features': FEATURES }
 BRANCHES = { BRANCH, ... }
 BRANCH = STRING : TYPE-REF
| STRING : { 'type': TYPE-REF, '*if': COND }

 Member 'union' names the union type.

 The 'base' member defines the common members.  If it is a MEMBERS_
 object, it defines common members just like a struct type's 'data'
 member defines struct type members.  If it is a STRING, it names a
 struct type whose members are the common members.

 Member 'discriminator' must name a non-optional enum-typed member of
 the base struct.  That member's value selects a branch by its name.
 If no such branch exists, an empty branch is assumed.

If I understand your commit message correctly, this paragraph is no
longer true.



Right. Like this:

Member 'discriminator' must name either a non-optional enum-typed member, or an 
enum type name. (and more description follow, about user defined function and 
so on).


Do you think that mixing member name and type name here is OK? Or should I 
instead add another field 'discriminator-type', so that exactly one of 
'discriminator' and 'discriminator-type' should be in union definition?




 Each BRANCH of the 'data' object defines a branch of the union.  A
 union must have at least one branch.

 The BRANCH's STRING name is the branch name.  It must be a value of
 the discriminator enum type.

 The BRANCH's value defines the branch's properties, in particular its
 type.  The type must a struct type.  The form TYPE-REF_ is shorthand
 for :code:`{ 'type': TYPE-REF }`.

 In the Client JSON Protocol, a union is represented by an object with
 the common members (from the base type) and the selected branch's
 members.  The two sets of member names must be disjoint.

 Example::

  { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
  { 'union': 'BlockdevOptions',
'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },
'discriminator': 'driver',
'data': { 'file': 'BlockdevOptionsFile',
  'qcow2': 'BlockdevOptionsQcow2' } }

 Resulting in these JSON objects::

  { "driver": "file", "read-only": true,
"filename": "/some/place/my-image" }
  { "driver": "qcow2", "read-only": false,
"backing": "/some/place/my-image", "lazy-refcounts": true }

 The order of branches need not match the order of the enum values.
 The branches need not cover all possible enum values.  In the
 resulting generated C data types, a union is represented as a struct
 with the base members in QAPI schema order, and then a union of
 structures for each branch of the struct.

 The optional 'if' member specifies a conditional.  See `Configuring
 the schema`_ below for more on this.

 The optional 'features' member specifies features.  See Features_
 below for more on this.



--
Best regards,
Vladimir




Re: [RFC 01/15] scripts/qapi: support type-based unions

2024-03-28 Thread Vladimir Sementsov-Ogievskiy

On 28.03.24 12:40, Markus Armbruster wrote:

Subject: all unions are type-based.  Perhaps "support implicit union
tags on the wire"?


Yes, sounds good.



Do you need this schema language feature for folding block jobs into the
jobs abstraction, or is it just for making the wire protocol nicer in
places?



It's not necessary, we can proceed with job-* API, specifying both type and id.

But I think, as we are not in a hurry, better to make new job-* API more 
effective from the beginning.

--
Best regards,
Vladimir




Re: [RFC 01/15] scripts/qapi: support type-based unions

2024-03-28 Thread Markus Armbruster
Subject: all unions are type-based.  Perhaps "support implicit union
tags on the wire"?

Do you need this schema language feature for folding block jobs into the
jobs abstraction, or is it just for making the wire protocol nicer in
places?




Re: [RFC 01/15] scripts/qapi: support type-based unions

2024-03-28 Thread Markus Armbruster
Vladimir Sementsov-Ogievskiy  writes:

> Look at block-job-change command: we have to specify both 'id' to chose
> the job to operate on and 'type' for QAPI union be parsed. But for user
> this looks redundant: when we specify 'id', QEMU should be able to get
> corresponding job's type.
>
> This commit brings such a possibility: just specify some Enum type as
> 'discriminator', and define a function somewhere with prototype
>
> bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)
>
> Further commits will use this functionality to upgrade block-job-change
> interface and introduce some new interfaces.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  scripts/qapi/introspect.py |  5 +++-
>  scripts/qapi/schema.py | 50 +++---
>  scripts/qapi/types.py  |  3 ++-
>  scripts/qapi/visit.py  | 43 ++--
>  4 files changed, 73 insertions(+), 28 deletions(-)

I believe you need to update docs/devel/qapi-code-gen.rst.

Current text:

Union types
---

Syntax::

UNION = { 'union': STRING,
  'base': ( MEMBERS | STRING ),
  'discriminator': STRING,
  'data': BRANCHES,
  '*if': COND,
  '*features': FEATURES }
BRANCHES = { BRANCH, ... }
BRANCH = STRING : TYPE-REF
   | STRING : { 'type': TYPE-REF, '*if': COND }

Member 'union' names the union type.

The 'base' member defines the common members.  If it is a MEMBERS_
object, it defines common members just like a struct type's 'data'
member defines struct type members.  If it is a STRING, it names a
struct type whose members are the common members.

Member 'discriminator' must name a non-optional enum-typed member of
the base struct.  That member's value selects a branch by its name.
If no such branch exists, an empty branch is assumed.

If I understand your commit message correctly, this paragraph is no
longer true.

Each BRANCH of the 'data' object defines a branch of the union.  A
union must have at least one branch.

The BRANCH's STRING name is the branch name.  It must be a value of
the discriminator enum type.

The BRANCH's value defines the branch's properties, in particular its
type.  The type must a struct type.  The form TYPE-REF_ is shorthand
for :code:`{ 'type': TYPE-REF }`.

In the Client JSON Protocol, a union is represented by an object with
the common members (from the base type) and the selected branch's
members.  The two sets of member names must be disjoint.

Example::

 { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
 { 'union': 'BlockdevOptions',
   'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },
   'discriminator': 'driver',
   'data': { 'file': 'BlockdevOptionsFile',
 'qcow2': 'BlockdevOptionsQcow2' } }

Resulting in these JSON objects::

 { "driver": "file", "read-only": true,
   "filename": "/some/place/my-image" }
 { "driver": "qcow2", "read-only": false,
   "backing": "/some/place/my-image", "lazy-refcounts": true }

The order of branches need not match the order of the enum values.
The branches need not cover all possible enum values.  In the
resulting generated C data types, a union is represented as a struct
with the base members in QAPI schema order, and then a union of
structures for each branch of the struct.

The optional 'if' member specifies a conditional.  See `Configuring
the schema`_ below for more on this.

The optional 'features' member specifies features.  See Features_
below for more on this.