* [PATCH v2 01/11] qapi: re-establish linting baseline
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-10-16 15:26 ` [PATCH v2 02/11] scripts: qapi: black format main.py Victor Toso
` (11 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
From: John Snow <jsnow@redhat.com>
Some very minor housekeeping to make the linters happy once more.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 2 +-
scripts/qapi/parser.py | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index bf5716b5f3..5412716617 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -13,8 +13,8 @@
from contextlib import contextmanager
import os
-import sys
import re
+import sys
from typing import (
Dict,
Iterator,
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 22e7bcc4b1..bf31018aef 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -22,6 +22,7 @@
Dict,
List,
Mapping,
+ Match,
Optional,
Set,
Union,
@@ -563,11 +564,11 @@ def end_comment(self) -> None:
self._switch_section(QAPIDoc.NullSection(self._parser))
@staticmethod
- def _match_at_name_colon(string: str):
+ def _match_at_name_colon(string: str) -> Optional[Match[str]]:
return re.match(r'@([^:]*): *', string)
@staticmethod
- def _match_section_tag(string: str):
+ def _match_section_tag(string: str) -> Optional[Match[str]]:
return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
def _append_body_line(self, line: str) -> None:
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
2023-10-16 15:26 ` [PATCH v2 01/11] qapi: re-establish linting baseline Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-10-18 11:00 ` Markus Armbruster
2023-10-16 15:26 ` [PATCH v2 03/11] qapi: golang: Generate qapi's enum types in Go Victor Toso
` (10 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
flake8 complained:
./main.py:60:1: E302 expected 2 blank lines, found 1
Which is simple enough. My vim has black [0] enabled by default, so it
did some extra formatting. I'm proposing to follow it.
[0] https://black.readthedocs.io/en/stable/
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
1 file changed, 48 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 316736b6a2..fe65c1a17a 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -22,18 +22,20 @@
def invalid_prefix_char(prefix: str) -> Optional[str]:
- match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ match = must_match(r"([A-Za-z_.-][A-Za-z0-9_.-]*)?", prefix)
if match.end() != len(prefix):
return prefix[match.end()]
return None
-def generate(schema_file: str,
- output_dir: str,
- prefix: str,
- unmask: bool = False,
- builtins: bool = False,
- gen_tracing: bool = False) -> None:
+def generate(
+ schema_file: str,
+ output_dir: str,
+ prefix: str,
+ unmask: bool = False,
+ builtins: bool = False,
+ gen_tracing: bool = False,
+) -> None:
"""
Generate C code for the given schema into the target directory.
@@ -63,25 +65,41 @@ def main() -> int:
:return: int, 0 on success, 1 on failure.
"""
parser = argparse.ArgumentParser(
- description='Generate code from a QAPI schema')
- parser.add_argument('-b', '--builtins', action='store_true',
- help="generate code for built-in types")
- parser.add_argument('-o', '--output-dir', action='store',
- default='',
- help="write output to directory OUTPUT_DIR")
- parser.add_argument('-p', '--prefix', action='store',
- default='',
- help="prefix for symbols")
- parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
- dest='unmask',
- help="expose non-ABI names in introspection")
+ description="Generate code from a QAPI schema"
+ )
+ parser.add_argument(
+ "-b",
+ "--builtins",
+ action="store_true",
+ help="generate code for built-in types",
+ )
+ parser.add_argument(
+ "-o",
+ "--output-dir",
+ action="store",
+ default="",
+ help="write output to directory OUTPUT_DIR",
+ )
+ parser.add_argument(
+ "-p", "--prefix", action="store", default="", help="prefix for symbols"
+ )
+ parser.add_argument(
+ "-u",
+ "--unmask-non-abi-names",
+ action="store_true",
+ dest="unmask",
+ help="expose non-ABI names in introspection",
+ )
# Option --suppress-tracing exists so we can avoid solving build system
# problems. TODO Drop it when we no longer need it.
- parser.add_argument('--suppress-tracing', action='store_true',
- help="suppress adding trace events to qmp marshals")
+ parser.add_argument(
+ "--suppress-tracing",
+ action="store_true",
+ help="suppress adding trace events to qmp marshals",
+ )
- parser.add_argument('schema', action='store')
+ parser.add_argument("schema", action="store")
args = parser.parse_args()
funny_char = invalid_prefix_char(args.prefix)
@@ -91,12 +109,14 @@ def main() -> int:
return 1
try:
- generate(args.schema,
- output_dir=args.output_dir,
- prefix=args.prefix,
- unmask=args.unmask,
- builtins=args.builtins,
- gen_tracing=not args.suppress_tracing)
+ generate(
+ args.schema,
+ output_dir=args.output_dir,
+ prefix=args.prefix,
+ unmask=args.unmask,
+ builtins=args.builtins,
+ gen_tracing=not args.suppress_tracing,
+ )
except QAPIError as err:
print(err, file=sys.stderr)
return 1
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-16 15:26 ` [PATCH v2 02/11] scripts: qapi: black format main.py Victor Toso
@ 2023-10-18 11:00 ` Markus Armbruster
2023-10-18 11:13 ` Daniel P. Berrangé
2023-10-18 15:23 ` Victor Toso
0 siblings, 2 replies; 43+ messages in thread
From: Markus Armbruster @ 2023-10-18 11:00 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
Victor Toso <victortoso@redhat.com> writes:
> flake8 complained:
> ./main.py:60:1: E302 expected 2 blank lines, found 1
>
> Which is simple enough. My vim has black [0] enabled by default, so it
> did some extra formatting. I'm proposing to follow it.
>
> [0] https://black.readthedocs.io/en/stable/
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
> scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
> 1 file changed, 48 insertions(+), 28 deletions(-)
Is this all black hates about scripts/qapi/?
Did you configure it in any way, and if yes, how?
[...]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-18 11:00 ` Markus Armbruster
@ 2023-10-18 11:13 ` Daniel P. Berrangé
2023-10-18 15:23 ` Victor Toso
1 sibling, 0 replies; 43+ messages in thread
From: Daniel P. Berrangé @ 2023-10-18 11:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Victor Toso, qemu-devel, John Snow, Andrea Bolognani
On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > flake8 complained:
> > ./main.py:60:1: E302 expected 2 blank lines, found 1
> >
> > Which is simple enough. My vim has black [0] enabled by default, so it
> > did some extra formatting. I'm proposing to follow it.
> >
> > [0] https://black.readthedocs.io/en/stable/
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
> > 1 file changed, 48 insertions(+), 28 deletions(-)
>
> Is this all black hates about scripts/qapi/?
>
> Did you configure it in any way, and if yes, how?
The point of the 'black' tool is that it be highly opinionated and
bring (force) all projects in the python code into following the
same style. As such it intentionally has near zero configuration
options.
You can control the line length it uses (88 by default) and something
related to string quoting style normalization, but that's basically it.
Generally though developers should just run 'black' and accept all the
changes it makes without questioning.
Personally I'd encourage the use of black for any project with python
code, precisely to remove any need to spend time debating code style.
If a project is also using flake8 there's a config needed for flake8
to stop it conflicting with black though
eg in $gitroot/.flake8 you'd need at least:
[flake8]
max-line-length = 88
extend-ignore = E203
If QEMU intends to adopt black, at the very least we need to have
it validated by CI and probably by 'make check' too, to avoid
regressions.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-18 11:00 ` Markus Armbruster
2023-10-18 11:13 ` Daniel P. Berrangé
@ 2023-10-18 15:23 ` Victor Toso
2023-10-19 5:42 ` Markus Armbruster
1 sibling, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-18 15:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
[-- Attachment #1: Type: text/plain, Size: 865 bytes --]
On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > flake8 complained:
> > ./main.py:60:1: E302 expected 2 blank lines, found 1
> >
> > Which is simple enough. My vim has black [0] enabled by default, so it
> > did some extra formatting. I'm proposing to follow it.
> >
> > [0] https://black.readthedocs.io/en/stable/
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
> > 1 file changed, 48 insertions(+), 28 deletions(-)
>
> Is this all black hates about scripts/qapi/?
No, just scripts/qapi/main.py.
> Did you configure it in any way, and if yes, how?
Only to reduce line length to 79.
I can do a separate series for this, if the idea is accepted.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-18 15:23 ` Victor Toso
@ 2023-10-19 5:42 ` Markus Armbruster
2023-10-19 7:30 ` Daniel P. Berrangé
0 siblings, 1 reply; 43+ messages in thread
From: Markus Armbruster @ 2023-10-19 5:42 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
Victor Toso <victortoso@redhat.com> writes:
> On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > flake8 complained:
>> > ./main.py:60:1: E302 expected 2 blank lines, found 1
>> >
>> > Which is simple enough. My vim has black [0] enabled by default, so it
>> > did some extra formatting. I'm proposing to follow it.
>> >
>> > [0] https://black.readthedocs.io/en/stable/
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
>> > 1 file changed, 48 insertions(+), 28 deletions(-)
>>
>> Is this all black hates about scripts/qapi/?
>
> No, just scripts/qapi/main.py.
>
>> Did you configure it in any way, and if yes, how?
>
> Only to reduce line length to 79.
>
> I can do a separate series for this, if the idea is accepted.
Let's build a rough idea of how much churn this would be.
We have a bit over 5000 lines:
$ wc -l scripts/qapi/*py
419 scripts/qapi/commands.py
251 scripts/qapi/common.py
50 scripts/qapi/error.py
251 scripts/qapi/events.py
679 scripts/qapi/expr.py
368 scripts/qapi/gen.py
390 scripts/qapi/introspect.py
103 scripts/qapi/main.py
777 scripts/qapi/parser.py
1233 scripts/qapi/schema.py
71 scripts/qapi/source.py
387 scripts/qapi/types.py
429 scripts/qapi/visit.py
5408 total
Feed them to black:
$ black -q -l 75 scripts/qapi
$ git-diff --stat
scripts/qapi/commands.py | 448 +++++++++++++++++-----------
scripts/qapi/common.py | 240 ++++++++++-----
scripts/qapi/error.py | 15 +-
scripts/qapi/events.py | 274 +++++++++++-------
scripts/qapi/expr.py | 409 ++++++++++++++++----------
scripts/qapi/gen.py | 187 +++++++-----
scripts/qapi/introspect.py | 323 +++++++++++++--------
scripts/qapi/main.py | 80 +++--
scripts/qapi/parser.py | 370 +++++++++++++----------
scripts/qapi/schema.py | 709 +++++++++++++++++++++++++++++----------------
scripts/qapi/source.py | 17 +-
scripts/qapi/types.py | 369 ++++++++++++++---------
scripts/qapi/visit.py | 355 ++++++++++++++---------
13 files changed, 2383 insertions(+), 1413 deletions(-)
*Ouch*
Peeking at the result, I see string quote normalization. Try again with
that switched off, and the line length relaxed:
$ black -q -l 79 -S scripts/qapi
$ git-diff --stat
scripts/qapi/commands.py | 357 +++++++++++++++++++++------------
scripts/qapi/common.py | 170 ++++++++++++----
scripts/qapi/error.py | 11 +-
scripts/qapi/events.py | 220 +++++++++++++-------
scripts/qapi/expr.py | 261 +++++++++++++++---------
scripts/qapi/gen.py | 114 ++++++-----
scripts/qapi/introspect.py | 231 +++++++++++++--------
scripts/qapi/main.py | 72 ++++---
scripts/qapi/parser.py | 224 ++++++++++++---------
scripts/qapi/schema.py | 488 +++++++++++++++++++++++++++++++--------------
scripts/qapi/source.py | 7 +-
scripts/qapi/types.py | 303 ++++++++++++++++++----------
scripts/qapi/visit.py | 287 ++++++++++++++++----------
13 files changed, 1802 insertions(+), 943 deletions(-)
Still massive churn.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 02/11] scripts: qapi: black format main.py
2023-10-19 5:42 ` Markus Armbruster
@ 2023-10-19 7:30 ` Daniel P. Berrangé
0 siblings, 0 replies; 43+ messages in thread
From: Daniel P. Berrangé @ 2023-10-19 7:30 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Victor Toso, qemu-devel, John Snow, Andrea Bolognani
On Thu, Oct 19, 2023 at 07:42:38AM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > flake8 complained:
> >> > ./main.py:60:1: E302 expected 2 blank lines, found 1
> >> >
> >> > Which is simple enough. My vim has black [0] enabled by default, so it
> >> > did some extra formatting. I'm proposing to follow it.
> >> >
> >> > [0] https://black.readthedocs.io/en/stable/
> >> >
> >> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> >> > ---
> >> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++----------------
> >> > 1 file changed, 48 insertions(+), 28 deletions(-)
> >>
> >> Is this all black hates about scripts/qapi/?
> >
> > No, just scripts/qapi/main.py.
> >
> >> Did you configure it in any way, and if yes, how?
> >
> > Only to reduce line length to 79.
> >
> > I can do a separate series for this, if the idea is accepted.
>
> Let's build a rough idea of how much churn this would be.
>
> We have a bit over 5000 lines:
>
> $ wc -l scripts/qapi/*py
> 419 scripts/qapi/commands.py
> 251 scripts/qapi/common.py
> 50 scripts/qapi/error.py
> 251 scripts/qapi/events.py
> 679 scripts/qapi/expr.py
> 368 scripts/qapi/gen.py
> 390 scripts/qapi/introspect.py
> 103 scripts/qapi/main.py
> 777 scripts/qapi/parser.py
> 1233 scripts/qapi/schema.py
> 71 scripts/qapi/source.py
> 387 scripts/qapi/types.py
> 429 scripts/qapi/visit.py
> 5408 total
>
> Feed them to black:
>
> $ black -q -l 75 scripts/qapi
> $ git-diff --stat
> scripts/qapi/commands.py | 448 +++++++++++++++++-----------
> scripts/qapi/common.py | 240 ++++++++++-----
> scripts/qapi/error.py | 15 +-
> scripts/qapi/events.py | 274 +++++++++++-------
> scripts/qapi/expr.py | 409 ++++++++++++++++----------
> scripts/qapi/gen.py | 187 +++++++-----
> scripts/qapi/introspect.py | 323 +++++++++++++--------
> scripts/qapi/main.py | 80 +++--
> scripts/qapi/parser.py | 370 +++++++++++++----------
> scripts/qapi/schema.py | 709 +++++++++++++++++++++++++++++----------------
> scripts/qapi/source.py | 17 +-
> scripts/qapi/types.py | 369 ++++++++++++++---------
> scripts/qapi/visit.py | 355 ++++++++++++++---------
> 13 files changed, 2383 insertions(+), 1413 deletions(-)
>
> *Ouch*
>
> Peeking at the result, I see string quote normalization. Try again with
> that switched off, and the line length relaxed:
>
> $ black -q -l 79 -S scripts/qapi
> $ git-diff --stat
> scripts/qapi/commands.py | 357 +++++++++++++++++++++------------
> scripts/qapi/common.py | 170 ++++++++++++----
> scripts/qapi/error.py | 11 +-
> scripts/qapi/events.py | 220 +++++++++++++-------
> scripts/qapi/expr.py | 261 +++++++++++++++---------
> scripts/qapi/gen.py | 114 ++++++-----
> scripts/qapi/introspect.py | 231 +++++++++++++--------
> scripts/qapi/main.py | 72 ++++---
> scripts/qapi/parser.py | 224 ++++++++++++---------
> scripts/qapi/schema.py | 488 +++++++++++++++++++++++++++++++--------------
> scripts/qapi/source.py | 7 +-
> scripts/qapi/types.py | 303 ++++++++++++++++++----------
> scripts/qapi/visit.py | 287 ++++++++++++++++----------
> 13 files changed, 1802 insertions(+), 943 deletions(-)
>
> Still massive churn.
FWIW, the .git-blame-ignore-revs file can be populated with commit
hashes afterwards, to make 'git blame' ignore the reformatting
changes. Doesn't help with people cherry-picking fixes to old
branches though, which is the other main downside of such churn.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 03/11] qapi: golang: Generate qapi's enum types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
2023-10-16 15:26 ` [PATCH v2 01/11] qapi: re-establish linting baseline Victor Toso
2023-10-16 15:26 ` [PATCH v2 02/11] scripts: qapi: black format main.py Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-10-16 15:26 ` [PATCH v2 04/11] qapi: golang: Generate qapi's alternate " Victor Toso
` (9 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI enum types and generates its equivalent in Go.
We sort the output based on enum's type name.
Enums are being handled as strings in Golang.
1. For each QAPI enum, we will define a string type in Go to be the
assigned type of this specific enum.
2. Naming: CamelCase will be used in any identifier that we want to
export, which is everything.
Example:
qapi:
| { 'enum': 'DisplayProtocol',
| 'data': [ 'vnc', 'spice' ] }
go:
| type DisplayProtocol string
|
| const (
| DisplayProtocolVnc DisplayProtocol = "vnc"
| DisplayProtocolSpice DisplayProtocol = "spice"
| )
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 173 +++++++++++++++++++++++++++++++++++++++++
scripts/qapi/main.py | 3 +
2 files changed, 176 insertions(+)
create mode 100644 scripts/qapi/golang.py
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
new file mode 100644
index 0000000000..dc12be7b03
--- /dev/null
+++ b/scripts/qapi/golang.py
@@ -0,0 +1,173 @@
+"""
+Golang QAPI generator
+"""
+# Copyright (c) 2023 Red Hat Inc.
+#
+# Authors:
+# Victor Toso <victortoso@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+# Just for type hint on self
+from __future__ import annotations
+
+import os
+from typing import List, Optional
+
+from .schema import (
+ QAPISchema,
+ QAPISchemaEnumMember,
+ QAPISchemaFeature,
+ QAPISchemaIfCond,
+ QAPISchemaObjectType,
+ QAPISchemaObjectTypeMember,
+ QAPISchemaType,
+ QAPISchemaVariants,
+ QAPISchemaVisitor,
+)
+from .source import QAPISourceInfo
+
+
+TEMPLATE_ENUM = """
+type {name} string
+
+const (
+{fields}
+)
+"""
+
+
+def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
+ vis = QAPISchemaGenGolangVisitor(prefix)
+ schema.visit(vis)
+ vis.write(output_dir)
+
+
+def qapi_to_field_name_enum(name: str) -> str:
+ return name.title().replace("-", "")
+
+
+def generate_content_from_dict(data: dict[str, str]) -> str:
+ content = ""
+
+ for name in sorted(data):
+ content += data[name]
+
+ return content
+
+
+class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
+ # pylint: disable=too-many-arguments
+ def __init__(self, _: str):
+ super().__init__()
+ types = ("enum",)
+ self.target = dict.fromkeys(types, "")
+ self.schema: QAPISchema
+ self.golang_package_name = "qapi"
+ self.enums: dict[str, str] = {}
+
+ def visit_begin(self, schema: QAPISchema) -> None:
+ self.schema = schema
+
+ # Every Go file needs to reference its package name
+ for target in self.target:
+ self.target[target] = f"package {self.golang_package_name}\n"
+
+ def visit_end(self) -> None:
+ del self.schema
+ self.target["enum"] += generate_content_from_dict(self.enums)
+
+ def visit_object_type(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ base: Optional[QAPISchemaObjectType],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants],
+ ) -> None:
+ pass
+
+ def visit_alternate_type(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ variants: QAPISchemaVariants,
+ ) -> None:
+ pass
+
+ def visit_enum_type(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ members: List[QAPISchemaEnumMember],
+ prefix: Optional[str],
+ ) -> None:
+ assert name not in self.enums
+
+ maxname = 0
+ for member in members:
+ enum_name = f"{name}{qapi_to_field_name_enum(member.name)}"
+ maxname = max(maxname, len(enum_name))
+
+ fields = ""
+ for member in members:
+ enum_name = f"{name}{qapi_to_field_name_enum(member.name)}"
+ name2type = " " * (maxname - len(enum_name) + 1)
+ fields += f"""\t{enum_name}{name2type}{name} = "{member.name}"\n"""
+
+ self.enums[name] = TEMPLATE_ENUM.format(name=name, fields=fields[:-1])
+
+ def visit_array_type(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ element_type: QAPISchemaType,
+ ) -> None:
+ pass
+
+ def visit_command(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ arg_type: Optional[QAPISchemaObjectType],
+ ret_type: Optional[QAPISchemaType],
+ gen: bool,
+ success_response: bool,
+ boxed: bool,
+ allow_oob: bool,
+ allow_preconfig: bool,
+ coroutine: bool,
+ ) -> None:
+ pass
+
+ def visit_event(
+ self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ arg_type: Optional[QAPISchemaObjectType],
+ boxed: bool,
+ ) -> None:
+ pass
+
+ def write(self, output_dir: str) -> None:
+ for module_name, content in self.target.items():
+ go_module = module_name + "s.go"
+ go_dir = "go"
+ pathname = os.path.join(output_dir, go_dir, go_module)
+ odir = os.path.dirname(pathname)
+ os.makedirs(odir, exist_ok=True)
+
+ with open(pathname, "w", encoding="utf8") as outfile:
+ outfile.write(content)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index fe65c1a17a..07c29bcbe9 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -15,6 +15,7 @@
from .common import must_match
from .error import QAPIError
from .events import gen_events
+from .golang import gen_golang
from .introspect import gen_introspect
from .schema import QAPISchema
from .types import gen_types
@@ -56,6 +57,8 @@ def generate(
gen_events(schema, output_dir, prefix)
gen_introspect(schema, output_dir, prefix, unmask)
+ gen_golang(schema, output_dir, prefix)
+
def main() -> int:
"""
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (2 preceding siblings ...)
2023-10-16 15:26 ` [PATCH v2 03/11] qapi: golang: Generate qapi's enum types in Go Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-11-06 15:28 ` Andrea Bolognani
2023-11-09 17:34 ` Andrea Bolognani
2023-10-16 15:26 ` [PATCH v2 05/11] qapi: golang: Generate qapi's struct " Victor Toso
` (8 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI alternate types and generates data structures
in Go that handles it.
Alternate types are similar to Union but without a discriminator that
can be used to identify the underlying value on the wire. It is needed
to infer it. In Go, most of the types [*] are mapped as optional
fields and Marshal and Unmarshal methods will be handling the data
checks.
Example:
qapi:
| { 'alternate': 'BlockdevRef',
| 'data': { 'definition': 'BlockdevOptions',
| 'reference': 'str' } }
go:
| type BlockdevRef struct {
| Definition *BlockdevOptions
| Reference *string
| }
usage:
| input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
| k := BlockdevRef{}
| err := json.Unmarshal([]byte(input), &k)
| if err != nil {
| panic(err)
| }
| // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
[*] The exception for optional fields as default is to Types that can
accept JSON Null as a value. For this case, we translate NULL to a
member type called IsNull, which is boolean in Go. This will be
explained better in the documentation patch of this series but the
main rationale is around Marshaling to and from JSON and Go data
structures.
Example:
qapi:
| { 'alternate': 'StrOrNull',
| 'data': { 's': 'str',
| 'n': 'null' } }
go:
| type StrOrNull struct {
| S *string
| IsNull bool
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 301 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 298 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index dc12be7b03..3f6692df4b 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -13,10 +13,11 @@
from __future__ import annotations
import os
-from typing import List, Optional
+from typing import List, Optional, Tuple
from .schema import (
QAPISchema,
+ QAPISchemaAlternateType,
QAPISchemaEnumMember,
QAPISchemaFeature,
QAPISchemaIfCond,
@@ -37,6 +38,77 @@
)
"""
+TEMPLATE_HELPER = """
+// Creates a decoder that errors on unknown Fields
+// Returns nil if successfully decoded @from payload to @into type
+// Returns error if failed to decode @from payload to @into type
+func StrictDecode(into interface{}, from []byte) error {
+\tdec := json.NewDecoder(strings.NewReader(string(from)))
+\tdec.DisallowUnknownFields()
+
+\tif err := dec.Decode(into); err != nil {
+\t\treturn err
+\t}
+\treturn nil
+}
+"""
+
+TEMPLATE_ALTERNATE = """
+// Only implemented on Alternate types that can take JSON NULL as value.
+//
+// This is a helper for the marshalling code. It should return true only when
+// the Alternate is empty (no members are set), otherwise it returns false and
+// the member set to be Marshalled.
+type AbsentAlternate interface {
+\tToAnyOrAbsent() (any, bool)
+}
+"""
+
+TEMPLATE_ALTERNATE_NULLABLE_CHECK = """
+\t\t}} else if s.{var_name} != nil {{
+\t\t\treturn *s.{var_name}, false"""
+
+TEMPLATE_ALTERNATE_MARSHAL_CHECK = """
+\tif s.{var_name} != nil {{
+\t\treturn json.Marshal(s.{var_name})
+\t}} else """
+
+TEMPLATE_ALTERNATE_UNMARSHAL_CHECK = """
+\t// Check for {var_type}
+\t{{
+\t\ts.{var_name} = new({var_type})
+\t\tif err := StrictDecode(s.{var_name}, data); err == nil {{
+\t\t\treturn nil
+\t\t}}
+\t\ts.{var_name} = nil
+\t}}
+"""
+
+TEMPLATE_ALTERNATE_NULLABLE = """
+func (s *{name}) ToAnyOrAbsent() (any, bool) {{
+\tif s != nil {{
+\t\tif s.IsNull {{
+\t\t\treturn nil, false
+{absent_check_fields}
+\t\t}}
+\t}}
+
+\treturn nil, true
+}}
+"""
+
+TEMPLATE_ALTERNATE_METHODS = """
+func (s {name}) MarshalJSON() ([]byte, error) {{
+\t{marshal_check_fields}
+\treturn {marshal_return_default}
+}}
+
+func (s *{name}) UnmarshalJSON(data []byte) error {{
+{unmarshal_check_fields}
+\treturn fmt.Errorf("Can't convert to {name}: %s", string(data))
+}}
+"""
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
@@ -44,10 +116,191 @@ def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis.write(output_dir)
+def qapi_to_field_name(name: str) -> str:
+ return name.title().replace("_", "").replace("-", "")
+
+
def qapi_to_field_name_enum(name: str) -> str:
return name.title().replace("-", "")
+def qapi_schema_type_to_go_type(qapitype: str) -> str:
+ schema_types_to_go = {
+ "str": "string",
+ "null": "nil",
+ "bool": "bool",
+ "number": "float64",
+ "size": "uint64",
+ "int": "int64",
+ "int8": "int8",
+ "int16": "int16",
+ "int32": "int32",
+ "int64": "int64",
+ "uint8": "uint8",
+ "uint16": "uint16",
+ "uint32": "uint32",
+ "uint64": "uint64",
+ "any": "any",
+ "QType": "QType",
+ }
+
+ prefix = ""
+ if qapitype.endswith("List"):
+ prefix = "[]"
+ qapitype = qapitype[:-4]
+
+ qapitype = schema_types_to_go.get(qapitype, qapitype)
+ return prefix + qapitype
+
+
+def qapi_field_to_go_field(
+ member_name: str, type_name: str
+) -> Tuple[str, str, str]:
+ # Nothing to generate on null types. We update some
+ # variables to handle json-null on marshalling methods.
+ if type_name == "null":
+ return "IsNull", "bool", ""
+
+ # This function is called on Alternate, so fields should be ptrs
+ return (
+ qapi_to_field_name(member_name),
+ qapi_schema_type_to_go_type(type_name),
+ "*",
+ )
+
+
+# Helper function for boxed or self contained structures.
+def generate_struct_type(
+ type_name, args: List[dict[str:str]] = None, ident: int = 0
+) -> str:
+ members = "{}"
+ base_ident = "\t" * ident
+ if args is not None:
+ # Most of the logic below is to mimic the gofmt tool.
+ # We calculate spaces between member and type and between
+ # the type and tag. Note that gofmt considers comments as
+ # divider between ident blocks.
+ maxname, maxtype = 0, 0
+ blocks: tuple(int, int) = []
+ for arg in args:
+ if "comment" in arg:
+ blocks.append((maxname, maxtype))
+ maxname, maxtype = 0, 0
+ continue
+
+ if "type" not in arg:
+ continue
+
+ maxname = max(maxname, len(arg["name"]))
+ maxtype = max(maxtype, len(arg["type"]))
+
+ blocks.append((maxname, maxtype))
+ block = 0
+
+ maxname, maxtype = blocks[0]
+ members = " {\n"
+ for arg in args:
+ if "comment" in arg:
+ block += 1
+ maxname, maxtype = blocks[block]
+ members += f"""\t// {arg["comment"]}\n"""
+ continue
+
+ name2type = ""
+ if "type" in arg:
+ name2type = " " * (maxname - len(arg["name"]) + 1)
+ type2tag = ""
+ if "tag" in arg:
+ type2tag = " " * (maxtype - len(arg["type"]) + 1)
+
+ fident = "\t" * (ident + 1)
+ gotype = "" if "type" not in arg else arg["type"]
+ tag = "" if "tag" not in arg else arg["tag"]
+ name = arg["name"]
+ members += (
+ f"""{fident}{name}{name2type}{gotype}{type2tag}{tag}\n"""
+ )
+ members += f"{base_ident}}}\n"
+
+ with_type = f"\n{base_ident}type {type_name}" if len(type_name) > 0 else ""
+ return f"""{with_type} struct{members}"""
+
+
+def generate_template_alternate(
+ self: QAPISchemaGenGolangVisitor,
+ name: str,
+ variants: Optional[QAPISchemaVariants],
+) -> str:
+ absent_check_fields = ""
+ args: List[dict[str:str]] = []
+ # to avoid having to check accept_null_types
+ nullable = False
+ if name in self.accept_null_types:
+ # In QEMU QAPI schema, only StrOrNull and BlockdevRefOrNull.
+ nullable = True
+ marshal_return_default = """[]byte("{}"), nil"""
+ marshal_check_fields = """if s.IsNull {
+\t\treturn []byte("null"), nil
+\t} else """
+ unmarshal_check_fields = """
+\t// Check for json-null first
+\tif string(data) == "null" {
+\t\ts.IsNull = true
+\t\treturn nil
+\t}"""
+ else:
+ marshal_return_default = f'nil, errors.New("{name} has empty fields")'
+ marshal_check_fields = ""
+ unmarshal_check_fields = f"""
+\t// Check for json-null first
+\tif string(data) == "null" {{
+\t\treturn errors.New(`null not supported for {name}`)
+\t}}"""
+
+ if variants:
+ for var in variants.variants:
+ var_name, var_type, isptr = qapi_field_to_go_field(
+ var.name, var.type.name
+ )
+ args.append(
+ {
+ "name": f"{var_name}",
+ "type": f"{isptr}{var_type}",
+ }
+ )
+
+ # Null is special, handled first
+ if var.type.name == "null":
+ assert nullable
+ continue
+
+ if nullable:
+ absent_check_fields += (
+ TEMPLATE_ALTERNATE_NULLABLE_CHECK.format(var_name=var_name)
+ )
+ marshal_check_fields += TEMPLATE_ALTERNATE_MARSHAL_CHECK[
+ 2:
+ ].format(var_name=var_name)
+ unmarshal_check_fields += (
+ TEMPLATE_ALTERNATE_UNMARSHAL_CHECK.format(
+ var_name=var_name, var_type=var_type
+ )
+ )
+
+ content = generate_struct_type(name, args)
+ if nullable:
+ content += TEMPLATE_ALTERNATE_NULLABLE.format(
+ name=name, absent_check_fields=absent_check_fields
+ )
+ content += TEMPLATE_ALTERNATE_METHODS.format(
+ name=name,
+ marshal_check_fields=marshal_check_fields[:-6],
+ marshal_return_default=marshal_return_default,
+ unmarshal_check_fields=unmarshal_check_fields[1:],
+ )
+ return content
+
+
def generate_content_from_dict(data: dict[str, str]) -> str:
content = ""
@@ -61,22 +314,60 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
# pylint: disable=too-many-arguments
def __init__(self, _: str):
super().__init__()
- types = ("enum",)
+ types = (
+ "alternate",
+ "enum",
+ "helper",
+ )
self.target = dict.fromkeys(types, "")
self.schema: QAPISchema
self.golang_package_name = "qapi"
self.enums: dict[str, str] = {}
+ self.alternates: dict[str, str] = {}
+ self.accept_null_types = []
def visit_begin(self, schema: QAPISchema) -> None:
self.schema = schema
+ # We need to be aware of any types that accept JSON NULL
+ for name, entity in self.schema._entity_dict.items():
+ if not isinstance(entity, QAPISchemaAlternateType):
+ # Assume that only Alternate types accept JSON NULL
+ continue
+
+ for var in entity.variants.variants:
+ if var.type.name == "null":
+ self.accept_null_types.append(name)
+ break
+
# Every Go file needs to reference its package name
+ # and most have some imports too.
for target in self.target:
self.target[target] = f"package {self.golang_package_name}\n"
+ if target == "helper":
+ imports = """\nimport (
+\t"encoding/json"
+\t"strings"
+)
+"""
+ else:
+ imports = """\nimport (
+\t"encoding/json"
+\t"errors"
+\t"fmt"
+)
+"""
+ if target != "enum":
+ self.target[target] += imports
+
+ self.target["helper"] += TEMPLATE_HELPER
+ self.target["alternate"] += TEMPLATE_ALTERNATE
+
def visit_end(self) -> None:
del self.schema
self.target["enum"] += generate_content_from_dict(self.enums)
+ self.target["alternate"] += generate_content_from_dict(self.alternates)
def visit_object_type(
self,
@@ -98,7 +389,11 @@ def visit_alternate_type(
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants,
) -> None:
- pass
+ assert name not in self.alternates
+
+ self.alternates[name] = generate_template_alternate(
+ self, name, variants
+ )
def visit_enum_type(
self,
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-10-16 15:26 ` [PATCH v2 04/11] qapi: golang: Generate qapi's alternate " Victor Toso
@ 2023-11-06 15:28 ` Andrea Bolognani
2023-11-06 15:52 ` Victor Toso
2023-11-09 17:34 ` Andrea Bolognani
1 sibling, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-06 15:28 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> Alternate types are similar to Union but without a discriminator that
> can be used to identify the underlying value on the wire. It is needed
> to infer it. In Go, most of the types [*] are mapped as optional
> fields and Marshal and Unmarshal methods will be handling the data
> checks.
>
> Example:
>
> qapi:
> | { 'alternate': 'BlockdevRef',
> | 'data': { 'definition': 'BlockdevOptions',
> | 'reference': 'str' } }
>
> go:
> | type BlockdevRef struct {
> | Definition *BlockdevOptions
> | Reference *string
> | }
>
> usage:
> | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
> | k := BlockdevRef{}
> | err := json.Unmarshal([]byte(input), &k)
> | if err != nil {
> | panic(err)
> | }
> | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
>
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go. This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.
These usage examples are great; in fact, I think they're too good to
be relegated to the commit messages. I would like to see them in the
actual documentation.
At the same time, the current documentation seems to focus a lot on
internals rather than usage. I think we really need two documents
here:
* one for users of the library, with lots of usage examples
(ideally at least one for JSON->Go and one for Go->JSON for each
class of QAPI object) and little-to-no peeking behind the
curtains;
* one for QEMU developers / QAPI maintainers, which goes into
detail regarding the internals and explains the various design
choices and trade-offs.
Some parts of the latter should probably be code comments instead. A
reasonable balance will have to be found.
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> +TEMPLATE_HELPER = """
> +// Creates a decoder that errors on unknown Fields
> +// Returns nil if successfully decoded @from payload to @into type
> +// Returns error if failed to decode @from payload to @into type
> +func StrictDecode(into interface{}, from []byte) error {
> +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> +\tdec.DisallowUnknownFields()
> +
> +\tif err := dec.Decode(into); err != nil {
> +\t\treturn err
> +\t}
> +\treturn nil
> +}
> +"""
I think the use of \t here makes things a lot less readable. Can't
you just do
TEMPLATE_HELPER = """
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
I would actually recommend the use of textwrap.dedent() to make
things less awkward:
TEMPLATE_HELPER = textwrap.dedent("""
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
This is particularly useful for blocks of Go code that are not
declared at the top level...
> + unmarshal_check_fields = f"""
> +\t// Check for json-null first
> +\tif string(data) == "null" {{
> +\t\treturn errors.New(`null not supported for {name}`)
> +\t}}"""
... such as this one, which could be turned into:
unmarshal_check_fields = textwrap.dedent(f"""
// Check for json-null first
if string(data) == "null" {{
return errors.New(`null not supported for {name}`)
}}
""")
Much more manageable, don't you think? :)
On a partially related note: while I haven't yet looked closely at
how much effort you've dedicated to producing pretty output, from a
quick look at generate_struct_type() it seems that the answer is "not
zero". I think it would be fine to simplify things there and produce
ugly output, under the assumption that gofmt will be called on the
generated code immediately afterwards. The C generator doesn't have
this luxury, but we should take advantage of it.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-11-06 15:28 ` Andrea Bolognani
@ 2023-11-06 15:52 ` Victor Toso
2023-11-06 16:12 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-11-06 15:52 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 5240 bytes --]
Hi,
On Mon, Nov 06, 2023 at 07:28:04AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> > This patch handles QAPI alternate types and generates data structures
> > in Go that handles it.
> >
> > Alternate types are similar to Union but without a discriminator that
> > can be used to identify the underlying value on the wire. It is needed
> > to infer it. In Go, most of the types [*] are mapped as optional
> > fields and Marshal and Unmarshal methods will be handling the data
> > checks.
> >
> > Example:
> >
> > qapi:
> > | { 'alternate': 'BlockdevRef',
> > | 'data': { 'definition': 'BlockdevOptions',
> > | 'reference': 'str' } }
> >
> > go:
> > | type BlockdevRef struct {
> > | Definition *BlockdevOptions
> > | Reference *string
> > | }
> >
> > usage:
> > | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
> > | k := BlockdevRef{}
> > | err := json.Unmarshal([]byte(input), &k)
> > | if err != nil {
> > | panic(err)
> > | }
> > | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
> >
> > [*] The exception for optional fields as default is to Types that can
> > accept JSON Null as a value. For this case, we translate NULL to a
> > member type called IsNull, which is boolean in Go. This will be
> > explained better in the documentation patch of this series but the
> > main rationale is around Marshaling to and from JSON and Go data
> > structures.
>
> These usage examples are great; in fact, I think they're too good to
> be relegated to the commit messages. I would like to see them in the
> actual documentation.
>
> At the same time, the current documentation seems to focus a lot on
> internals rather than usage. I think we really need two documents
> here:
>
> * one for users of the library, with lots of usage examples
> (ideally at least one for JSON->Go and one for Go->JSON for each
> class of QAPI object) and little-to-no peeking behind the
> curtains;
>
> * one for QEMU developers / QAPI maintainers, which goes into
> detail regarding the internals and explains the various design
> choices and trade-offs.
>
> Some parts of the latter should probably be code comments instead. A
> reasonable balance will have to be found.
>
> > diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> > +TEMPLATE_HELPER = """
> > +// Creates a decoder that errors on unknown Fields
> > +// Returns nil if successfully decoded @from payload to @into type
> > +// Returns error if failed to decode @from payload to @into type
> > +func StrictDecode(into interface{}, from []byte) error {
> > +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> > +\tdec.DisallowUnknownFields()
> > +
> > +\tif err := dec.Decode(into); err != nil {
> > +\t\treturn err
> > +\t}
> > +\treturn nil
> > +}
> > +"""
>
> I think the use of \t here makes things a lot less readable. Can't
> you just do
>
> TEMPLATE_HELPER = """
> func StrictDecode() {
> dec := ...
> if err := ... {
> return err
> }
> return nil
> }
> """
>
> I would actually recommend the use of textwrap.dedent() to make
> things less awkward:
>
> TEMPLATE_HELPER = textwrap.dedent("""
> func StrictDecode() {
> dec := ...
> if err := ... {
> return err
> }
> return nil
> }
> """
>
> This is particularly useful for blocks of Go code that are not
> declared at the top level...
>
> > + unmarshal_check_fields = f"""
> > +\t// Check for json-null first
> > +\tif string(data) == "null" {{
> > +\t\treturn errors.New(`null not supported for {name}`)
> > +\t}}"""
>
> ... such as this one, which could be turned into:
>
> unmarshal_check_fields = textwrap.dedent(f"""
> // Check for json-null first
> if string(data) == "null" {{
> return errors.New(`null not supported for {name}`)
> }}
> """)
>
> Much more manageable, don't you think? :)
Didn't know this one, I agree 100% that is nicer. I'll take a
look for the next iteration if the output would still be similar
as I want to gofmt change be zero (see bellow).
>
> On a partially related note: while I haven't yet looked closely at
> how much effort you've dedicated to producing pretty output, from a
> quick look at generate_struct_type() it seems that the answer is "not
> zero". I think it would be fine to simplify things there and produce
> ugly output, under the assumption that gofmt will be called on the
> generated code immediately afterwards. The C generator doesn't have
> this luxury, but we should take advantage of it.
Yes, I wholeheartedly agree. The idea of the generator producing
a well formatted Go code came from previous review. I didn't want
to introduce gofmt and friends to QEMU build system, perhaps it
wasn't a big deal but I find it foreign to QEMU for a generated
code that QEMU itself would not use.
See: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01188.html
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-11-06 15:52 ` Victor Toso
@ 2023-11-06 16:12 ` Andrea Bolognani
0 siblings, 0 replies; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-06 16:12 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Nov 06, 2023 at 04:52:12PM +0100, Victor Toso wrote:
> On Mon, Nov 06, 2023 at 07:28:04AM -0800, Andrea Bolognani wrote:
> > On a partially related note: while I haven't yet looked closely at
> > how much effort you've dedicated to producing pretty output, from a
> > quick look at generate_struct_type() it seems that the answer is "not
> > zero". I think it would be fine to simplify things there and produce
> > ugly output, under the assumption that gofmt will be called on the
> > generated code immediately afterwards. The C generator doesn't have
> > this luxury, but we should take advantage of it.
>
> Yes, I wholeheartedly agree. The idea of the generator producing
> a well formatted Go code came from previous review. I didn't want
> to introduce gofmt and friends to QEMU build system, perhaps it
> wasn't a big deal but I find it foreign to QEMU for a generated
> code that QEMU itself would not use.
>
> See: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01188.html
Noted.
Whether or not requiring a pass through gofmt is an issue kind of
depends on how we end up shipping these files.
What seems the most probable to me is that we'll have a separate repo
where we dump the generated files and that Go users will consume via
a regular 'import'. Your existing qapi-go repo follows this model. In
this context, gofmt is never going to be called as part of the QEMU
build process so it doesn't really matter.
But maybe there was a discussion around this that I've missed :)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-10-16 15:26 ` [PATCH v2 04/11] qapi: golang: Generate qapi's alternate " Victor Toso
2023-11-06 15:28 ` Andrea Bolognani
@ 2023-11-09 17:34 ` Andrea Bolognani
2023-11-09 19:01 ` Victor Toso
1 sibling, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-09 17:34 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go. This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.
>
> Example:
>
> qapi:
> | { 'alternate': 'StrOrNull',
> | 'data': { 's': 'str',
> | 'n': 'null' } }
>
> go:
> | type StrOrNull struct {
> | S *string
> | IsNull bool
> | }
We should call this Null instead of IsNull, and also make it a
pointer similarly to what I just suggested for union branches that
don't have additional data attached to them.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-11-09 17:34 ` Andrea Bolognani
@ 2023-11-09 19:01 ` Victor Toso
2023-11-10 9:58 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-11-09 19:01 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]
Hi,
On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> > [*] The exception for optional fields as default is to Types that can
> > accept JSON Null as a value. For this case, we translate NULL to a
> > member type called IsNull, which is boolean in Go. This will be
> > explained better in the documentation patch of this series but the
> > main rationale is around Marshaling to and from JSON and Go data
> > structures.
> >
> > Example:
> >
> > qapi:
> > | { 'alternate': 'StrOrNull',
> > | 'data': { 's': 'str',
> > | 'n': 'null' } }
> >
> > go:
> > | type StrOrNull struct {
> > | S *string
> > | IsNull bool
> > | }
>
> We should call this Null instead of IsNull, and also make it a
> pointer similarly to what I just suggested for union branches
> that don't have additional data attached to them.
I don't have a strong argument here against what you suggest, I
just think that the usage of bool is simpler.
The test I have here [0] would have code changing to something
like:
When is null:
- val := &StrOrNull{IsNull: true}
+ myNull := JSONNull{}
+ val := &StrOrNull{Null: &myNull}
So you need a instance to get a pointer.
When is absent (no fields set), no changes as both bool defaults
to false and pointer to nil.
The code handling this would change from:
- if u.IsNull {
+ if u.Null != nil {
...
}
We don't have the same issues as Union, under the hood we Marshal
to/Unmarshal from "null" and that would not change.
[0] https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
I can change this in the next iteration.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-11-09 19:01 ` Victor Toso
@ 2023-11-10 9:58 ` Andrea Bolognani
2023-11-10 15:43 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-10 9:58 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Thu, Nov 09, 2023 at 08:01:48PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> > We should call this Null instead of IsNull, and also make it a
> > pointer similarly to what I just suggested for union branches
> > that don't have additional data attached to them.
>
> I don't have a strong argument here against what you suggest, I
> just think that the usage of bool is simpler.
>
> The test I have here [0] would have code changing to something
> like:
>
> When is null:
>
> - val := &StrOrNull{IsNull: true}
> + myNull := JSONNull{}
> + val := &StrOrNull{Null: &myNull}
>
> So you need a instance to get a pointer.
>
> When is absent (no fields set), no changes as both bool defaults
> to false and pointer to nil.
>
> The code handling this would change from:
>
> - if u.IsNull {
> + if u.Null != nil {
> ...
> }
>
> We don't have the same issues as Union, under the hood we Marshal
> to/Unmarshal from "null" and that would not change.
>
> [0] https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
>
> I can change this in the next iteration.
No, leave the type alone. But I still think the name should probably
be Null instead of IsNull.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
2023-11-10 9:58 ` Andrea Bolognani
@ 2023-11-10 15:43 ` Victor Toso
0 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-11-10 15:43 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
Hi,
On Fri, Nov 10, 2023 at 01:58:20AM -0800, Andrea Bolognani wrote:
> On Thu, Nov 09, 2023 at 08:01:48PM +0100, Victor Toso wrote:
> > On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> > > We should call this Null instead of IsNull, and also make it a
> > > pointer similarly to what I just suggested for union branches
> > > that don't have additional data attached to them.
> >
> > I don't have a strong argument here against what you suggest, I
> > just think that the usage of bool is simpler.
> >
> > The test I have here [0] would have code changing to something
> > like:
> >
> > When is null:
> >
> > - val := &StrOrNull{IsNull: true}
> > + myNull := JSONNull{}
> > + val := &StrOrNull{Null: &myNull}
> >
> > So you need a instance to get a pointer.
> >
> > When is absent (no fields set), no changes as both bool defaults
> > to false and pointer to nil.
> >
> > The code handling this would change from:
> >
> > - if u.IsNull {
> > + if u.Null != nil {
> > ...
> > }
> >
> > We don't have the same issues as Union, under the hood we Marshal
> > to/Unmarshal from "null" and that would not change.
> >
> > [0] https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
> >
> > I can change this in the next iteration.
>
> No, leave the type alone. But I still think the name should probably
> be Null instead of IsNull.
Ok, keeping bool, rename to Null. Deal.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 05/11] qapi: golang: Generate qapi's struct types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (3 preceding siblings ...)
2023-10-16 15:26 ` [PATCH v2 04/11] qapi: golang: Generate qapi's alternate " Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-10-16 15:26 ` [PATCH v2 06/11] qapi: golang: structs: Address 'null' members Victor Toso
` (7 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI struct types and generates the equivalent
types in Go. The following patch adds extra logic when a member of the
struct has a Type that can take JSON Null value (e.g: StrOrNull in
QEMU)
The highlights of this implementation are:
1. Generating an Go struct that requires a @base type, the @base type
fields are copied over to the Go struct. The advantage of this
approach is to not have embed structs in any of the QAPI types.
Note that embedding a @base type is recursive, that is, if the
@base type has a @base, all of those fields will be copied over.
2. About the Go struct's fields:
i) They can be either by Value or Reference.
ii) Every field that is marked as optional in the QAPI specification
are translated to Reference fields in its Go structure. This
design decision is the most straightforward way to check if a
given field was set or not. Exception only for types that can
take JSON Null value.
iii) Mandatory fields are always by Value with the exception of QAPI
arrays, which are handled by Reference (to a block of memory) by
Go.
iv) All the fields are named with Uppercase due Golang's export
convention.
v) In order to avoid any kind of issues when encoding or decoding,
to or from JSON, we mark all fields with its @name and, when it is
optional, member, with @omitempty
Example:
qapi:
| { 'struct': 'BlockdevCreateOptionsFile',
| 'data': { 'filename': 'str',
| 'size': 'size',
| '*preallocation': 'PreallocMode',
| '*nocow': 'bool',
| '*extent-size-hint': 'size'} }
go:
| type BlockdevCreateOptionsFile struct {
| Filename string `json:"filename"`
| Size uint64 `json:"size"`
| Preallocation *PreallocMode `json:"preallocation,omitempty"`
| Nocow *bool `json:"nocow,omitempty"`
| ExtentSizeHint *uint64 `json:"extent-size-hint,omitempty"`
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 156 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 155 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 3f6692df4b..73d0804d0a 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -116,6 +116,14 @@ def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis.write(output_dir)
+def qapi_name_is_base(name: str) -> bool:
+ return qapi_name_is_object(name) and name.endswith("-base")
+
+
+def qapi_name_is_object(name: str) -> bool:
+ return name.startswith("q_obj_")
+
+
def qapi_to_field_name(name: str) -> str:
return name.title().replace("_", "").replace("-", "")
@@ -124,6 +132,24 @@ def qapi_to_field_name_enum(name: str) -> str:
return name.title().replace("-", "")
+def qapi_to_go_type_name(name: str) -> str:
+ if qapi_name_is_object(name):
+ name = name[6:]
+
+ # We want to keep CamelCase for Golang types. We want to avoid removing
+ # already set CameCase names while fixing uppercase ones, eg:
+ # 1) q_obj_SocketAddress_base -> SocketAddressBase
+ # 2) q_obj_WATCHDOG-arg -> WatchdogArg
+ words = list(name.replace("_", "-").split("-"))
+ name = words[0]
+ if name.islower() or name.isupper():
+ name = name.title()
+
+ name += "".join(word.title() for word in words[1:])
+
+ return name
+
+
def qapi_schema_type_to_go_type(qapitype: str) -> str:
schema_types_to_go = {
"str": "string",
@@ -226,6 +252,98 @@ def generate_struct_type(
return f"""{with_type} struct{members}"""
+def get_struct_field(
+ self: QAPISchemaGenGolangVisitor,
+ qapi_name: str,
+ qapi_type_name: str,
+ is_optional: bool,
+ is_variant: bool,
+) -> dict[str:str]:
+
+ field = qapi_to_field_name(qapi_name)
+ member_type = qapi_schema_type_to_go_type(qapi_type_name)
+
+ optional = ""
+ if is_optional:
+ if member_type not in self.accept_null_types:
+ optional = ",omitempty"
+
+ # Use pointer to type when field is optional
+ isptr = "*" if is_optional and member_type[0] not in "*[" else ""
+
+ fieldtag = (
+ '`json:"-"`' if is_variant else f'`json:"{qapi_name}{optional}"`'
+ )
+ arg = {
+ "name": f"{field}",
+ "type": f"{isptr}{member_type}",
+ "tag": f"{fieldtag}",
+ }
+ return arg
+
+
+def recursive_base(
+ self: QAPISchemaGenGolangVisitor,
+ base: Optional[QAPISchemaObjectType],
+ discriminator: Optional[str] = None,
+) -> List[dict[str:str]]:
+ fields: List[dict[str:str]] = []
+
+ if not base:
+ return fields
+
+ if base.base is not None:
+ embed_base = self.schema.lookup_entity(base.base.name)
+ fields = recursive_base(self, embed_base, discriminator)
+
+ for member in base.local_members:
+ if discriminator and member.name == discriminator:
+ continue
+ field = get_struct_field(
+ self, member.name, member.type.name, member.optional, False
+ )
+ fields.append(field)
+
+ return fields
+
+
+# Helper function that is used for most of QAPI types
+def qapi_to_golang_struct(
+ self: QAPISchemaGenGolangVisitor,
+ name: str,
+ _: Optional[QAPISourceInfo],
+ __: QAPISchemaIfCond,
+ ___: List[QAPISchemaFeature],
+ base: Optional[QAPISchemaObjectType],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants],
+) -> str:
+
+ fields = recursive_base(self, base)
+
+ if members:
+ for member in members:
+ field = get_struct_field(
+ self, member.name, member.type.name, member.optional, False
+ )
+ fields.append(field)
+
+ if variants:
+ fields.append({"comment": "Variants fields"})
+ for variant in variants.variants:
+ if variant.type.is_implicit():
+ continue
+
+ field = get_struct_field(
+ self, variant.name, variant.type.name, True, True
+ )
+ fields.append(field)
+
+ type_name = qapi_to_go_type_name(name)
+ content = generate_struct_type(type_name, fields)
+ return content
+
+
def generate_template_alternate(
self: QAPISchemaGenGolangVisitor,
name: str,
@@ -318,12 +436,14 @@ def __init__(self, _: str):
"alternate",
"enum",
"helper",
+ "struct",
)
self.target = dict.fromkeys(types, "")
self.schema: QAPISchema
self.golang_package_name = "qapi"
self.enums: dict[str, str] = {}
self.alternates: dict[str, str] = {}
+ self.structs: dict[str, str] = {}
self.accept_null_types = []
def visit_begin(self, schema: QAPISchema) -> None:
@@ -368,6 +488,7 @@ def visit_end(self) -> None:
del self.schema
self.target["enum"] += generate_content_from_dict(self.enums)
self.target["alternate"] += generate_content_from_dict(self.alternates)
+ self.target["struct"] += generate_content_from_dict(self.structs)
def visit_object_type(
self,
@@ -379,7 +500,40 @@ def visit_object_type(
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants],
) -> None:
- pass
+ # Do not handle anything besides struct.
+ if (
+ name == self.schema.the_empty_object_type.name
+ or not isinstance(name, str)
+ or info.defn_meta not in ["struct"]
+ ):
+ return
+
+ # Base structs are embed
+ if qapi_name_is_base(name):
+ return
+
+ # Safety checks.
+ assert name not in self.structs
+
+ # visit all inner objects as well, they are not going to be
+ # called by python's generator.
+ if variants:
+ for var in variants.variants:
+ assert isinstance(var.type, QAPISchemaObjectType)
+ self.visit_object_type(
+ self,
+ var.type.name,
+ var.type.info,
+ var.type.ifcond,
+ var.type.base,
+ var.type.local_members,
+ var.type.variants,
+ )
+
+ # Save generated Go code to be written later
+ self.structs[name] = qapi_to_golang_struct(
+ self, name, info, ifcond, features, base, members, variants
+ )
def visit_alternate_type(
self,
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 06/11] qapi: golang: structs: Address 'null' members
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (4 preceding siblings ...)
2023-10-16 15:26 ` [PATCH v2 05/11] qapi: golang: Generate qapi's struct " Victor Toso
@ 2023-10-16 15:26 ` Victor Toso
2023-10-16 15:27 ` [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go Victor Toso
` (6 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:26 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
Explaining why this is needed needs some context, so taking the
example of StrOrNull alternate type and considering a simplified
struct that has two fields:
qapi:
| { 'struct': 'MigrationExample',
| 'data': { '*label': 'StrOrNull',
| 'target': 'StrOrNull' } }
We have a optional member 'label' which can have three JSON values:
1. A string: { "target": "a.host.com", "label": "happy" }
2. A null : { "target": "a.host.com", "label": null }
3. Absent : { "target": null}
The member 'target' is not optional, hence it can't be absent.
A Go struct that contains a optional type that can be JSON Null like
'label' in the example above, will need extra care when Marshaling and
Unmarshaling from JSON.
This patch handles this very specific case:
- It implements the Marshaler interface for these structs to properly
handle these values.
- It adds the interface AbsentAlternate() and implement it for any
Alternate that can be JSON Null
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 243 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 228 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 73d0804d0a..6a7e37dd90 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -110,6 +110,26 @@
"""
+TEMPLATE_STRUCT_WITH_NULLABLE_MARSHAL = """
+func (s {type_name}) MarshalJSON() ([]byte, error) {{
+\tm := make(map[string]any)
+{map_members}{map_special}
+\treturn json.Marshal(&m)
+}}
+
+func (s *{type_name}) UnmarshalJSON(data []byte) error {{
+\ttmp := {struct}{{}}
+
+\tif err := json.Unmarshal(data, &tmp); err != nil {{
+\t\treturn err
+\t}}
+
+{set_members}{set_special}
+\treturn nil
+}}
+"""
+
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
schema.visit(vis)
@@ -256,21 +276,31 @@ def get_struct_field(
self: QAPISchemaGenGolangVisitor,
qapi_name: str,
qapi_type_name: str,
+ within_nullable_struct: bool,
is_optional: bool,
is_variant: bool,
-) -> dict[str:str]:
+) -> Tuple[dict[str:str], bool]:
field = qapi_to_field_name(qapi_name)
member_type = qapi_schema_type_to_go_type(qapi_type_name)
+ is_nullable = False
optional = ""
if is_optional:
- if member_type not in self.accept_null_types:
+ if member_type in self.accept_null_types:
+ is_nullable = True
+ else:
optional = ",omitempty"
# Use pointer to type when field is optional
isptr = "*" if is_optional and member_type[0] not in "*[" else ""
+ if within_nullable_struct:
+ # Within a struct which has a field of type that can hold JSON NULL,
+ # we have to _not_ use a pointer, otherwise the Marshal methods are
+ # not called.
+ isptr = "" if member_type in self.accept_null_types else isptr
+
fieldtag = (
'`json:"-"`' if is_variant else f'`json:"{qapi_name}{optional}"`'
)
@@ -279,32 +309,202 @@ def get_struct_field(
"type": f"{isptr}{member_type}",
"tag": f"{fieldtag}",
}
- return arg
+ return arg, is_nullable
+
+
+# This helper is used whithin a struct that has members that accept JSON NULL.
+def map_and_set(
+ is_nullable: bool, field: str, field_is_optional: bool, name: str
+) -> Tuple[str, str]:
+
+ mapstr = ""
+ setstr = ""
+ if is_nullable:
+ mapstr = f"""
+\tif val, absent := s.{field}.ToAnyOrAbsent(); !absent {{
+\t\tm["{name}"] = val
+\t}}
+"""
+ setstr += f"""
+\tif _, absent := (&tmp.{field}).ToAnyOrAbsent(); !absent {{
+\t\ts.{field} = &tmp.{field}
+\t}}
+"""
+ elif field_is_optional:
+ mapstr = f"""
+\tif s.{field} != nil {{
+\t\tm["{name}"] = s.{field}
+\t}}
+"""
+ setstr = f"""\ts.{field} = tmp.{field}\n"""
+ else:
+ mapstr = f"""\tm["{name}"] = s.{field}\n"""
+ setstr = f"""\ts.{field} = tmp.{field}\n"""
+
+ return mapstr, setstr
+
+
+def recursive_base_nullable(
+ self: QAPISchemaGenGolangVisitor, base: Optional[QAPISchemaObjectType]
+) -> Tuple[List[dict[str:str]], str, str, str, str]:
+ fields: List[dict[str:str]] = []
+ map_members = ""
+ set_members = ""
+ map_special = ""
+ set_special = ""
+
+ if not base:
+ return fields, map_members, set_members, map_special, set_special
+
+ if base.base is not None:
+ embed_base = self.schema.lookup_entity(base.base.name)
+ (
+ fields,
+ map_members,
+ set_members,
+ map_special,
+ set_special,
+ ) = recursive_base_nullable(self, embed_base)
+
+ for member in base.local_members:
+ field, _ = get_struct_field(
+ self, member.name, member.type.name, True, member.optional, False
+ )
+ fields.append(field)
+
+ member_type = qapi_schema_type_to_go_type(member.type.name)
+ nullable = member_type in self.accept_null_types
+ field_name = qapi_to_field_name(member.name)
+ tomap, toset = map_and_set(
+ nullable, field_name, member.optional, member.name
+ )
+ if nullable:
+ map_special += tomap
+ set_special += toset
+ else:
+ map_members += tomap
+ set_members += toset
+
+ return fields, map_members, set_members, map_special, set_special
+
+
+# Helper function. This is executed when the QAPI schema has members
+# that could accept JSON NULL (e.g: StrOrNull in QEMU"s QAPI schema).
+# This struct will need to be extended with Marshal/Unmarshal methods to
+# properly handle such atypical members.
+#
+# Only the Marshallaing methods are generated but we do need to iterate over
+# all the members to properly set/check them in those methods.
+def struct_with_nullable_generate_marshal(
+ self: QAPISchemaGenGolangVisitor,
+ name: str,
+ base: Optional[QAPISchemaObjectType],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants],
+) -> str:
+
+ (
+ fields,
+ map_members,
+ set_members,
+ map_special,
+ set_special,
+ ) = recursive_base_nullable(self, base)
+
+ if members:
+ for member in members:
+ field, _ = get_struct_field(
+ self,
+ member.name,
+ member.type.name,
+ True,
+ member.optional,
+ False,
+ )
+ fields.append(field)
+
+ member_type = qapi_schema_type_to_go_type(member.type.name)
+ nullable = member_type in self.accept_null_types
+ tomap, toset = map_and_set(
+ nullable,
+ qapi_to_field_name(member.name),
+ member.optional,
+ member.name,
+ )
+ if nullable:
+ map_special += tomap
+ set_special += toset
+ else:
+ map_members += tomap
+ set_members += toset
+
+ if variants:
+ for variant in variants.variants:
+ if variant.type.is_implicit():
+ continue
+
+ field, _ = get_struct_field(
+ self,
+ variant.name,
+ variant.type.name,
+ True,
+ variant.optional,
+ True,
+ )
+ fields.append(field)
+
+ member_type = qapi_schema_type_to_go_type(variant.type.name)
+ nullable = member_type in self.accept_null_types
+ tomap, toset = map_and_set(
+ nullable,
+ qapi_to_field_name(variant.name),
+ variant.optional,
+ variant.name,
+ )
+ if nullable:
+ map_special += tomap
+ set_special += toset
+ else:
+ map_members += tomap
+ set_members += toset
+
+ type_name = qapi_to_go_type_name(name)
+ struct = generate_struct_type("", fields, ident=1)[:-1]
+ return TEMPLATE_STRUCT_WITH_NULLABLE_MARSHAL.format(
+ struct=struct[1:],
+ type_name=type_name,
+ map_members=map_members,
+ map_special=map_special,
+ set_members=set_members,
+ set_special=set_special,
+ )
def recursive_base(
self: QAPISchemaGenGolangVisitor,
base: Optional[QAPISchemaObjectType],
discriminator: Optional[str] = None,
-) -> List[dict[str:str]]:
+) -> Tuple[List[dict[str:str]], bool]:
fields: List[dict[str:str]] = []
+ with_nullable = False
if not base:
- return fields
+ return fields, with_nullable
if base.base is not None:
embed_base = self.schema.lookup_entity(base.base.name)
- fields = recursive_base(self, embed_base, discriminator)
+ fields, with_nullable = recursive_base(self, embed_base, discriminator)
for member in base.local_members:
if discriminator and member.name == discriminator:
continue
- field = get_struct_field(
- self, member.name, member.type.name, member.optional, False
+ field, nullable = get_struct_field(
+ self, member.name, member.type.name, False, member.optional, False
)
fields.append(field)
+ with_nullable = True if nullable else with_nullable
- return fields
+ return fields, with_nullable
# Helper function that is used for most of QAPI types
@@ -319,14 +519,20 @@ def qapi_to_golang_struct(
variants: Optional[QAPISchemaVariants],
) -> str:
- fields = recursive_base(self, base)
+ fields, with_nullable = recursive_base(self, base)
if members:
for member in members:
- field = get_struct_field(
- self, member.name, member.type.name, member.optional, False
+ field, nullable = get_struct_field(
+ self,
+ member.name,
+ member.type.name,
+ False,
+ member.optional,
+ False,
)
fields.append(field)
+ with_nullable = True if nullable else with_nullable
if variants:
fields.append({"comment": "Variants fields"})
@@ -334,13 +540,18 @@ def qapi_to_golang_struct(
if variant.type.is_implicit():
continue
- field = get_struct_field(
- self, variant.name, variant.type.name, True, True
+ field, nullable = get_struct_field(
+ self, variant.name, variant.type.name, False, True, True
)
fields.append(field)
+ with_nullable = True if nullable else with_nullable
type_name = qapi_to_go_type_name(name)
content = generate_struct_type(type_name, fields)
+ if with_nullable:
+ content += struct_with_nullable_generate_marshal(
+ self, name, base, members, variants
+ )
return content
@@ -465,7 +676,9 @@ def visit_begin(self, schema: QAPISchema) -> None:
for target in self.target:
self.target[target] = f"package {self.golang_package_name}\n"
- if target == "helper":
+ if target == "struct":
+ imports = '\nimport "encoding/json"\n'
+ elif target == "helper":
imports = """\nimport (
\t"encoding/json"
\t"strings"
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (5 preceding siblings ...)
2023-10-16 15:26 ` [PATCH v2 06/11] qapi: golang: structs: Address 'null' members Victor Toso
@ 2023-10-16 15:27 ` Victor Toso
2023-11-09 17:29 ` Andrea Bolognani
2023-10-16 15:27 ` [PATCH v2 08/11] qapi: golang: Generate qapi's event " Victor Toso
` (5 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI union types and generates the equivalent data
structures and methods in Go to handle it.
The QAPI union type has two types of fields: The @base and the
@Variants members. The @base fields can be considered common members
for the union while only one field maximum is set for the @Variants.
In the QAPI specification, it defines a @discriminator field, which is
an Enum type. The purpose of the @discriminator is to identify which
@variant type is being used.
For the @discriminator's enum that are not handled by the QAPI Union,
we add in the Go struct a separate block as "Unbranched enum fields".
The rationale for this extra block is to allow the user to pass that
enum value under the discriminator, without extra payload.
The union types implement the Marshaler and Unmarshaler interfaces to
seamless decode from JSON objects to Golang structs and vice versa.
qapi:
| { 'union': 'SetPasswordOptions',
| 'base': { 'protocol': 'DisplayProtocol',
| 'password': 'str',
| '*connected': 'SetPasswordAction' },
| 'discriminator': 'protocol',
| 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
go:
| type SetPasswordOptions struct {
| Password string `json:"password"`
| Connected *SetPasswordAction `json:"connected,omitempty"`
| // Variants fields
| Vnc *SetPasswordOptionsVnc `json:"-"`
| // Unbranched enum fields
| Spice bool `json:"-"`
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 228 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 217 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 6a7e37dd90..bc6206797a 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -51,6 +51,17 @@
\t}
\treturn nil
}
+
+// This helper is used to move struct's fields into a map.
+// This function is useful to merge JSON objects.
+func unwrapToMap(m map[string]any, data any) error {
+\tif bytes, err := json.Marshal(&data); err != nil {
+\t\treturn fmt.Errorf("unwrapToMap: %s", err)
+\t} else if err := json.Unmarshal(bytes, &m); err != nil {
+\t\treturn fmt.Errorf("unwrapToMap: %s, data=%s", err, string(bytes))
+\t}
+\treturn nil
+}
"""
TEMPLATE_ALTERNATE = """
@@ -130,6 +141,81 @@
"""
+TEMPLATE_UNION_CHECK_VARIANT_FIELD = """
+\tif s.{field} != nil && err == nil {{
+\t\tif len(bytes) != 0 {{
+\t\t\terr = errors.New(`multiple variant fields set`)
+\t\t}} else if err = unwrapToMap(m, s.{field}); err == nil {{
+\t\t\tm["{discriminator}"] = {go_enum_value}
+\t\t\tbytes, err = json.Marshal(m)
+\t\t}}
+\t}}
+"""
+
+TEMPLATE_UNION_CHECK_UNBRANCHED_FIELD = """
+\tif s.{field} && err == nil {{
+\t\tif len(bytes) != 0 {{
+\t\t\terr = errors.New(`multiple variant fields set`)
+\t\t}} else {{
+\t\t\tm["{discriminator}"] = {go_enum_value}
+\t\t\tbytes, err = json.Marshal(m)
+\t\t}}
+\t}}
+"""
+
+TEMPLATE_UNION_DRIVER_VARIANT_CASE = """
+\tcase {go_enum_value}:
+\t\ts.{field} = new({member_type})
+\t\tif err := json.Unmarshal(data, s.{field}); err != nil {{
+\t\t\ts.{field} = nil
+\t\t\treturn err
+\t\t}}"""
+
+TEMPLATE_UNION_DRIVER_UNBRANCHED_CASE = """
+\tcase {go_enum_value}:
+\t\ts.{field} = true
+"""
+
+TEMPLATE_UNION_METHODS = """
+func (s {type_name}) MarshalJSON() ([]byte, error) {{
+\tvar bytes []byte
+\tvar err error
+\tm := make(map[string]any)
+\t{{
+\t\ttype Alias {type_name}
+\t\tv := Alias(s)
+\t\tunwrapToMap(m, &v)
+\t}}
+{check_fields}
+\tif err != nil {{
+\t\treturn nil, fmt.Errorf("marshal {type_name} due:'%s' struct='%+v'", err, s)
+\t}} else if len(bytes) == 0 {{
+\t\treturn nil, fmt.Errorf("marshal {type_name} unsupported, struct='%+v'", s)
+\t}}
+\treturn bytes, nil
+}}
+
+func (s *{type_name}) UnmarshalJSON(data []byte) error {{
+{base_type_def}
+\ttmp := struct {{
+\t\t{base_type_name}
+\t}}{{}}
+
+\tif err := json.Unmarshal(data, &tmp); err != nil {{
+\t\treturn err
+\t}}
+{base_type_assign_unmarshal}
+\tswitch tmp.{discriminator} {{
+{driver_cases}
+\tdefault:
+\t\treturn fmt.Errorf("unmarshal {type_name} received unrecognized value '%s'",
+\t\t\ttmp.{discriminator})
+\t}}
+\treturn nil
+}}
+"""
+
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
schema.visit(vis)
@@ -511,15 +597,17 @@ def recursive_base(
def qapi_to_golang_struct(
self: QAPISchemaGenGolangVisitor,
name: str,
- _: Optional[QAPISourceInfo],
+ info: Optional[QAPISourceInfo],
__: QAPISchemaIfCond,
___: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants],
+ ident: int = 0,
) -> str:
- fields, with_nullable = recursive_base(self, base)
+ discriminator = None if not variants else variants.tag_member.name
+ fields, with_nullable = recursive_base(self, base, discriminator)
if members:
for member in members:
@@ -534,20 +622,37 @@ def qapi_to_golang_struct(
fields.append(field)
with_nullable = True if nullable else with_nullable
+ exists = {}
if variants:
fields.append({"comment": "Variants fields"})
for variant in variants.variants:
if variant.type.is_implicit():
continue
+ exists[variant.name] = True
field, nullable = get_struct_field(
self, variant.name, variant.type.name, False, True, True
)
fields.append(field)
with_nullable = True if nullable else with_nullable
+ if info.defn_meta == "union" and variants:
+ enum_name = variants.tag_member.type.name
+ enum_obj = self.schema.lookup_entity(enum_name)
+ if len(exists) != len(enum_obj.members):
+ fields.append({"comment": "Unbranched enum fields"})
+ for member in enum_obj.members:
+ if member.name in exists:
+ continue
+
+ field, nullable = get_struct_field(
+ self, member.name, "bool", False, False, True
+ )
+ fields.append(field)
+ with_nullable = True if nullable else with_nullable
+
type_name = qapi_to_go_type_name(name)
- content = generate_struct_type(type_name, fields)
+ content = generate_struct_type(type_name, fields, ident)
if with_nullable:
content += struct_with_nullable_generate_marshal(
self, name, base, members, variants
@@ -555,6 +660,96 @@ def qapi_to_golang_struct(
return content
+def qapi_to_golang_methods_union(
+ self: QAPISchemaGenGolangVisitor,
+ name: str,
+ base: Optional[QAPISchemaObjectType],
+ variants: Optional[QAPISchemaVariants],
+) -> str:
+
+ type_name = qapi_to_go_type_name(name)
+
+ assert base
+ base_type_assign_unmarshal = ""
+ base_type_name = qapi_to_go_type_name(base.name)
+ base_type_def = qapi_to_golang_struct(
+ self,
+ base.name,
+ base.info,
+ base.ifcond,
+ base.features,
+ base.base,
+ base.members,
+ base.variants,
+ ident=1,
+ )
+
+ discriminator = qapi_to_field_name(variants.tag_member.name)
+ for member in base.local_members:
+ field = qapi_to_field_name(member.name)
+ if field == discriminator:
+ continue
+ base_type_assign_unmarshal += f"""
+\ts.{field} = tmp.{field}"""
+
+ driver_cases = ""
+ check_fields = ""
+ exists = {}
+ enum_name = variants.tag_member.type.name
+ if variants:
+ for var in variants.variants:
+ if var.type.is_implicit():
+ continue
+
+ field = qapi_to_field_name(var.name)
+ enum_value = qapi_to_field_name_enum(var.name)
+ member_type = qapi_schema_type_to_go_type(var.type.name)
+ go_enum_value = f"""{enum_name}{enum_value}"""
+ exists[go_enum_value] = True
+
+ check_fields += TEMPLATE_UNION_CHECK_VARIANT_FIELD.format(
+ field=field,
+ discriminator=variants.tag_member.name,
+ go_enum_value=go_enum_value,
+ )
+ driver_cases += TEMPLATE_UNION_DRIVER_VARIANT_CASE.format(
+ go_enum_value=go_enum_value,
+ field=field,
+ member_type=member_type,
+ )
+
+ enum_obj = self.schema.lookup_entity(enum_name)
+ if len(exists) != len(enum_obj.members):
+ for member in enum_obj.members:
+ value = qapi_to_field_name_enum(member.name)
+ go_enum_value = f"""{enum_name}{value}"""
+
+ if go_enum_value in exists:
+ continue
+
+ field = qapi_to_field_name(member.name)
+
+ check_fields += TEMPLATE_UNION_CHECK_UNBRANCHED_FIELD.format(
+ field=field,
+ discriminator=variants.tag_member.name,
+ go_enum_value=go_enum_value,
+ )
+ driver_cases += TEMPLATE_UNION_DRIVER_UNBRANCHED_CASE.format(
+ go_enum_value=go_enum_value,
+ field=field,
+ )
+
+ return TEMPLATE_UNION_METHODS.format(
+ type_name=type_name,
+ check_fields=check_fields[1:],
+ base_type_def=base_type_def[1:],
+ base_type_name=base_type_name,
+ base_type_assign_unmarshal=base_type_assign_unmarshal,
+ discriminator=discriminator,
+ driver_cases=driver_cases[1:],
+ )
+
+
def generate_template_alternate(
self: QAPISchemaGenGolangVisitor,
name: str,
@@ -648,6 +843,7 @@ def __init__(self, _: str):
"enum",
"helper",
"struct",
+ "union",
)
self.target = dict.fromkeys(types, "")
self.schema: QAPISchema
@@ -655,6 +851,7 @@ def __init__(self, _: str):
self.enums: dict[str, str] = {}
self.alternates: dict[str, str] = {}
self.structs: dict[str, str] = {}
+ self.unions: dict[str, str] = {}
self.accept_null_types = []
def visit_begin(self, schema: QAPISchema) -> None:
@@ -681,6 +878,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
elif target == "helper":
imports = """\nimport (
\t"encoding/json"
+\t"fmt"
\t"strings"
)
"""
@@ -702,6 +900,7 @@ def visit_end(self) -> None:
self.target["enum"] += generate_content_from_dict(self.enums)
self.target["alternate"] += generate_content_from_dict(self.alternates)
self.target["struct"] += generate_content_from_dict(self.structs)
+ self.target["union"] += generate_content_from_dict(self.unions)
def visit_object_type(
self,
@@ -713,11 +912,11 @@ def visit_object_type(
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants],
) -> None:
- # Do not handle anything besides struct.
+ # Do not handle anything besides struct and unions.
if (
name == self.schema.the_empty_object_type.name
or not isinstance(name, str)
- or info.defn_meta not in ["struct"]
+ or info.defn_meta not in ["struct", "union"]
):
return
@@ -725,9 +924,6 @@ def visit_object_type(
if qapi_name_is_base(name):
return
- # Safety checks.
- assert name not in self.structs
-
# visit all inner objects as well, they are not going to be
# called by python's generator.
if variants:
@@ -744,9 +940,19 @@ def visit_object_type(
)
# Save generated Go code to be written later
- self.structs[name] = qapi_to_golang_struct(
- self, name, info, ifcond, features, base, members, variants
- )
+ if info.defn_meta == "struct":
+ assert name not in self.structs
+ self.structs[name] = qapi_to_golang_struct(
+ self, name, info, ifcond, features, base, members, variants
+ )
+ else:
+ assert name not in self.unions
+ self.unions[name] = qapi_to_golang_struct(
+ self, name, info, ifcond, features, base, members, variants
+ )
+ self.unions[name] += qapi_to_golang_methods_union(
+ self, name, base, variants
+ )
def visit_alternate_type(
self,
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
2023-10-16 15:27 ` [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go Victor Toso
@ 2023-11-09 17:29 ` Andrea Bolognani
2023-11-09 18:35 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-09 17:29 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:27:00PM +0200, Victor Toso wrote:
> This patch handles QAPI union types and generates the equivalent data
> structures and methods in Go to handle it.
>
> The QAPI union type has two types of fields: The @base and the
> @Variants members. The @base fields can be considered common members
> for the union while only one field maximum is set for the @Variants.
>
> In the QAPI specification, it defines a @discriminator field, which is
> an Enum type. The purpose of the @discriminator is to identify which
> @variant type is being used.
>
> For the @discriminator's enum that are not handled by the QAPI Union,
> we add in the Go struct a separate block as "Unbranched enum fields".
> The rationale for this extra block is to allow the user to pass that
> enum value under the discriminator, without extra payload.
>
> The union types implement the Marshaler and Unmarshaler interfaces to
> seamless decode from JSON objects to Golang structs and vice versa.
>
> qapi:
> | { 'union': 'SetPasswordOptions',
> | 'base': { 'protocol': 'DisplayProtocol',
> | 'password': 'str',
> | '*connected': 'SetPasswordAction' },
> | 'discriminator': 'protocol',
> | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
>
> go:
> | type SetPasswordOptions struct {
> | Password string `json:"password"`
> | Connected *SetPasswordAction `json:"connected,omitempty"`
> | // Variants fields
> | Vnc *SetPasswordOptionsVnc `json:"-"`
> | // Unbranched enum fields
> | Spice bool `json:"-"`
> | }
Instead of using bool for these, can we denote a special type? For
example
type Empty struct{}
We could then do
u := SetPasswordOptions{
Password: "...",
Spice: &Empty{},
}
The benefit I have in mind is that you'd be able to check which
variant field is set consistently:
if u.Vnc != nil {
...
}
if u.Spice != nil {
...
}
Additionally, this would allow client code that *looks* at the union
to keep working even if actual data is later added to the branch;
client code that *creates* the union would need to be updated, of
course, but that would be the case regardless.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
2023-11-09 17:29 ` Andrea Bolognani
@ 2023-11-09 18:35 ` Victor Toso
2023-11-10 9:54 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-11-09 18:35 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 3301 bytes --]
Hi,
On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:27:00PM +0200, Victor Toso wrote:
> > This patch handles QAPI union types and generates the equivalent data
> > structures and methods in Go to handle it.
> >
> > The QAPI union type has two types of fields: The @base and the
> > @Variants members. The @base fields can be considered common members
> > for the union while only one field maximum is set for the @Variants.
> >
> > In the QAPI specification, it defines a @discriminator field, which is
> > an Enum type. The purpose of the @discriminator is to identify which
> > @variant type is being used.
> >
> > For the @discriminator's enum that are not handled by the QAPI Union,
> > we add in the Go struct a separate block as "Unbranched enum fields".
> > The rationale for this extra block is to allow the user to pass that
> > enum value under the discriminator, without extra payload.
> >
> > The union types implement the Marshaler and Unmarshaler interfaces to
> > seamless decode from JSON objects to Golang structs and vice versa.
> >
> > qapi:
> > | { 'union': 'SetPasswordOptions',
> > | 'base': { 'protocol': 'DisplayProtocol',
> > | 'password': 'str',
> > | '*connected': 'SetPasswordAction' },
> > | 'discriminator': 'protocol',
> > | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
> >
> > go:
> > | type SetPasswordOptions struct {
> > | Password string `json:"password"`
> > | Connected *SetPasswordAction `json:"connected,omitempty"`
> > | // Variants fields
> > | Vnc *SetPasswordOptionsVnc `json:"-"`
> > | // Unbranched enum fields
> > | Spice bool `json:"-"`
> > | }
>
> Instead of using bool for these, can we denote a special type? For
> example
>
> type Empty struct{}
>
> We could then do
>
> u := SetPasswordOptions{
> Password: "...",
> Spice: &Empty{},
> }
>
> The benefit I have in mind is that you'd be able to check which
> variant field is set consistently:
>
> if u.Vnc != nil {
> ...
> }
> if u.Spice != nil {
> ...
> }
>
> Additionally, this would allow client code that *looks* at the
> union to keep working even if actual data is later added to the
> branch; client code that *creates* the union would need to be
> updated, of course, but that would be the case regardless.
I think it is better to not have code that is working to keep
working in this case where Spice is implemented.
Implementing Spice here would mean that a struct type
SetPasswordOptionsSpice was created but because the code handling
it before was using struct type Empty, it will not handle the new
struct, leading to possible runtime errors (e.g: not handling
username/password)
A bool would be simpler, triggering compile time errors.
Note that I'm working with the mindset that each version of the
module talks well with a version of QEMU. We should consider next
if we want to implement logic for QAPI versioning promises, which
is for more than one QEMU version. Markus had an email about it
last year. Daniel also suggested more version promises than what
QAPI currently does.
Anyway, that's my rationale for bool here.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
2023-11-09 18:35 ` Victor Toso
@ 2023-11-10 9:54 ` Andrea Bolognani
2023-11-10 15:45 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-10 9:54 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Thu, Nov 09, 2023 at 07:35:04PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote:
> > Additionally, this would allow client code that *looks* at the
> > union to keep working even if actual data is later added to the
> > branch; client code that *creates* the union would need to be
> > updated, of course, but that would be the case regardless.
>
> I think it is better to not have code that is working to keep
> working in this case where Spice is implemented.
>
> Implementing Spice here would mean that a struct type
> SetPasswordOptionsSpice was created but because the code handling
> it before was using struct type Empty, it will not handle the new
> struct, leading to possible runtime errors (e.g: not handling
> username/password)
>
> A bool would be simpler, triggering compile time errors.
You've convinced me :) Let's leave it like this for now. Once we
start seriously thinking about compatibility across versions then we
might have to reconsider this, but it's going to be part of a much
bigger, much more fun conversation ;)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
2023-11-10 9:54 ` Andrea Bolognani
@ 2023-11-10 15:45 ` Victor Toso
0 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-11-10 15:45 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]
Hi,
On Fri, Nov 10, 2023 at 01:54:50AM -0800, Andrea Bolognani wrote:
> On Thu, Nov 09, 2023 at 07:35:04PM +0100, Victor Toso wrote:
> > On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote:
> > > Additionally, this would allow client code that *looks* at the
> > > union to keep working even if actual data is later added to the
> > > branch; client code that *creates* the union would need to be
> > > updated, of course, but that would be the case regardless.
> >
> > I think it is better to not have code that is working to keep
> > working in this case where Spice is implemented.
> >
> > Implementing Spice here would mean that a struct type
> > SetPasswordOptionsSpice was created but because the code handling
> > it before was using struct type Empty, it will not handle the new
> > struct, leading to possible runtime errors (e.g: not handling
> > username/password)
> >
> > A bool would be simpler, triggering compile time errors.
>
> You've convinced me :) Let's leave it like this for now. Once
> we start seriously thinking about compatibility across versions
> then we might have to reconsider this, but it's going to be
> part of a much bigger, much more fun conversation ;)
Yes! I'm looking forward to a 'unstable' version where we can
agree on building things on top.
Thanks,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (6 preceding siblings ...)
2023-10-16 15:27 ` [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go Victor Toso
@ 2023-10-16 15:27 ` Victor Toso
2023-11-09 17:59 ` Andrea Bolognani
2023-10-16 15:27 ` [PATCH v2 09/11] qapi: golang: Generate qapi's command " Victor Toso
` (4 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI event types and generates data structures in
Go that handles it.
We also define a Event interface and two helper functions MarshalEvent
and UnmarshalEvent.
Example:
qapi:
| { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
| 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
go:
| type MemoryDeviceSizeChangeEvent struct {
| MessageTimestamp Timestamp `json:"-"`
| Id *string `json:"id,omitempty"`
| Size uint64 `json:"size"`
| QomPath string `json:"qom-path"`
| }
usage:
| input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
| `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
| `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
| e, err := UnmarshalEvent([]byte(input)
| if err != nil {
| panic(err)
| }
| if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
| m := e.(*MemoryDeviceSizeChangeEvent)
| // m.QomPath == "/machine/unattached/device[2]"
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 122 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index bc6206797a..81b320d6dd 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -215,6 +215,54 @@
}}
"""
+TEMPLATE_EVENT = """
+type Timestamp struct {{
+\tSeconds int64 `json:"seconds"`
+\tMicroseconds int64 `json:"microseconds"`
+}}
+
+type Event interface {{
+\tGetName() string
+\tGetTimestamp() Timestamp
+}}
+
+func MarshalEvent(e Event) ([]byte, error) {{
+\tm := make(map[string]any)
+\tm["event"] = e.GetName()
+\tm["timestamp"] = e.GetTimestamp()
+\tif bytes, err := json.Marshal(e); err != nil {{
+\t\treturn []byte{{}}, err
+\t}} else if len(bytes) > 2 {{
+\t\tm["data"] = e
+\t}}
+\treturn json.Marshal(m)
+}}
+
+func UnmarshalEvent(data []byte) (Event, error) {{
+\tbase := struct {{
+\t\tName string `json:"event"`
+\t\tMessageTimestamp Timestamp `json:"timestamp"`
+\t}}{{}}
+\tif err := json.Unmarshal(data, &base); err != nil {{
+\t\treturn nil, fmt.Errorf("Failed to decode event: %s", string(data))
+\t}}
+
+\tswitch base.Name {{{cases}
+\t}}
+\treturn nil, errors.New("Failed to recognize event")
+}}
+"""
+
+TEMPLATE_EVENT_METHODS = """
+func (s *{type_name}) GetName() string {{
+\treturn "{name}"
+}}
+
+func (s *{type_name}) GetTimestamp() Timestamp {{
+\treturn s.MessageTimestamp
+}}
+"""
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
@@ -238,7 +286,7 @@ def qapi_to_field_name_enum(name: str) -> str:
return name.title().replace("-", "")
-def qapi_to_go_type_name(name: str) -> str:
+def qapi_to_go_type_name(name: str, meta: Optional[str] = None) -> str:
if qapi_name_is_object(name):
name = name[6:]
@@ -253,6 +301,11 @@ def qapi_to_go_type_name(name: str) -> str:
name += "".join(word.title() for word in words[1:])
+ types = ["event"]
+ if meta in types:
+ name = name[:-3] if name.endswith("Arg") else name
+ name += meta.title().replace(" ", "")
+
return name
@@ -608,6 +661,15 @@ def qapi_to_golang_struct(
discriminator = None if not variants else variants.tag_member.name
fields, with_nullable = recursive_base(self, base, discriminator)
+ if info.defn_meta == "event":
+ fields.insert(
+ 0,
+ {
+ "name": "MessageTimestamp",
+ "type": "Timestamp",
+ "tag": """`json:"-"`""",
+ },
+ )
if members:
for member in members:
@@ -651,7 +713,7 @@ def qapi_to_golang_struct(
fields.append(field)
with_nullable = True if nullable else with_nullable
- type_name = qapi_to_go_type_name(name)
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
content = generate_struct_type(type_name, fields, ident)
if with_nullable:
content += struct_with_nullable_generate_marshal(
@@ -825,6 +887,28 @@ def generate_template_alternate(
return content
+def generate_template_event(events: dict[str, Tuple[str, str]]) -> str:
+ cases = ""
+ content = ""
+ for name in sorted(events):
+ case_type, gocode = events[name]
+ content += gocode
+ cases += f"""
+\tcase "{name}":
+\t\tevent := struct {{
+\t\t\tData {case_type} `json:"data"`
+\t\t}}{{}}
+
+\t\tif err := json.Unmarshal(data, &event); err != nil {{
+\t\t\treturn nil, fmt.Errorf("Failed to unmarshal: %s", string(data))
+\t\t}}
+\t\tevent.Data.MessageTimestamp = base.MessageTimestamp
+\t\treturn &event.Data, nil
+"""
+ content += TEMPLATE_EVENT.format(cases=cases)
+ return content
+
+
def generate_content_from_dict(data: dict[str, str]) -> str:
content = ""
@@ -841,12 +925,14 @@ def __init__(self, _: str):
types = (
"alternate",
"enum",
+ "event",
"helper",
"struct",
"union",
)
self.target = dict.fromkeys(types, "")
self.schema: QAPISchema
+ self.events: dict[str, Tuple[str, str]] = {}
self.golang_package_name = "qapi"
self.enums: dict[str, str] = {}
self.alternates: dict[str, str] = {}
@@ -901,6 +987,7 @@ def visit_end(self) -> None:
self.target["alternate"] += generate_content_from_dict(self.alternates)
self.target["struct"] += generate_content_from_dict(self.structs)
self.target["union"] += generate_content_from_dict(self.unions)
+ self.target["event"] += generate_template_event(self.events)
def visit_object_type(
self,
@@ -1027,7 +1114,36 @@ def visit_event(
arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
) -> None:
- pass
+ assert name == info.defn_name
+ assert name not in self.events
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+
+ if isinstance(arg_type, QAPISchemaObjectType):
+ content = qapi_to_golang_struct(
+ self,
+ name,
+ arg_type.info,
+ arg_type.ifcond,
+ arg_type.features,
+ arg_type.base,
+ arg_type.members,
+ arg_type.variants,
+ )
+ else:
+ args: List[dict[str:str]] = []
+ args.append(
+ {
+ "name": "MessageTimestamp",
+ "type": "Timestamp",
+ "tag": """`json:"-"`""",
+ }
+ )
+ content = generate_struct_type(type_name, args)
+
+ content += TEMPLATE_EVENT_METHODS.format(
+ name=name, type_name=type_name
+ )
+ self.events[name] = (type_name, content)
def write(self, output_dir: str) -> None:
for module_name, content in self.target.items():
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
2023-10-16 15:27 ` [PATCH v2 08/11] qapi: golang: Generate qapi's event " Victor Toso
@ 2023-11-09 17:59 ` Andrea Bolognani
2023-11-09 19:13 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-09 17:59 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:27:01PM +0200, Victor Toso wrote:
> This patch handles QAPI event types and generates data structures in
> Go that handles it.
>
> We also define a Event interface and two helper functions MarshalEvent
> and UnmarshalEvent.
>
> Example:
> qapi:
> | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>
> go:
> | type MemoryDeviceSizeChangeEvent struct {
> | MessageTimestamp Timestamp `json:"-"`
> | Id *string `json:"id,omitempty"`
> | Size uint64 `json:"size"`
> | QomPath string `json:"qom-path"`
> | }
>
> usage:
> | input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
> | `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
> | `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
> | e, err := UnmarshalEvent([]byte(input)
> | if err != nil {
> | panic(err)
> | }
> | if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
> | m := e.(*MemoryDeviceSizeChangeEvent)
> | // m.QomPath == "/machine/unattached/device[2]"
> | }
I don't think we should encourage people to perform string
comparisons, as it completely sidesteps Go's type system and is thus
error-prone. Safer version:
switch m := e.(type) {
case *MemoryDeviceSizeChangeEvent:
// m.QomPath == "/machine/unattached/device[2]"
}
Now, I'm not sure I would go as far as suggesting that the GetName()
function should be completely removed, but maybe we can try leaving
it out from the initial version and see if people start screaming?
API-wise, I'm not a fan of the fact that we're forcing users to call
(Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
something like
func GetEventType(data []byte) (Event, error) {
type event struct {
Name string `json:"event"`
}
tmp := event{}
if err := json.Unmarshal(data, &tmp); err != nil {
return nil, err
}
switch tmp.Name {
case "MEMORY_DEVICE_SIZE_CHANGE":
return &MemoryDeviceSizeChangeEvent{}, nil
...
}
return nil, fmt.Errorf("unrecognized event '%s'", tmp.Name)
}
it becomes feasible to stick with standard functions. We can of
course keep the (Un)MarshalEvent functions around for convenience,
but I don't think they should be the only available API.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
2023-11-09 17:59 ` Andrea Bolognani
@ 2023-11-09 19:13 ` Victor Toso
2023-11-10 9:52 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-11-09 19:13 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]
Hi,
On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:27:01PM +0200, Victor Toso wrote:
> > This patch handles QAPI event types and generates data structures in
> > Go that handles it.
> >
> > We also define a Event interface and two helper functions MarshalEvent
> > and UnmarshalEvent.
> >
> > Example:
> > qapi:
> > | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> > | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
> >
> > go:
> > | type MemoryDeviceSizeChangeEvent struct {
> > | MessageTimestamp Timestamp `json:"-"`
> > | Id *string `json:"id,omitempty"`
> > | Size uint64 `json:"size"`
> > | QomPath string `json:"qom-path"`
> > | }
> >
> > usage:
> > | input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
> > | `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
> > | `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
> > | e, err := UnmarshalEvent([]byte(input)
> > | if err != nil {
> > | panic(err)
> > | }
> > | if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
> > | m := e.(*MemoryDeviceSizeChangeEvent)
> > | // m.QomPath == "/machine/unattached/device[2]"
> > | }
>
> I don't think we should encourage people to perform string
> comparisons, as it completely sidesteps Go's type system and is
> thus error-prone. Safer version:
>
> switch m := e.(type) {
> case *MemoryDeviceSizeChangeEvent:
> // m.QomPath == "/machine/unattached/device[2]"
> }
I agree.
> Now, I'm not sure I would go as far as suggesting that the
> GetName() function should be completely removed, but maybe we
> can try leaving it out from the initial version and see if
> people start screaming?
It might be useful for debugging too. I would rather log
e.GetName() than the string version of the type but if that's the
only reason we needed, I agree on removing for now.
> API-wise, I'm not a fan of the fact that we're forcing users to call
> (Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
> something like
>
> func GetEventType(data []byte) (Event, error) {
> type event struct {
> Name string `json:"event"`
> }
>
> tmp := event{}
> if err := json.Unmarshal(data, &tmp); err != nil {
> return nil, err
> }
>
> switch tmp.Name {
> case "MEMORY_DEVICE_SIZE_CHANGE":
> return &MemoryDeviceSizeChangeEvent{}, nil
> ...
> }
>
> return nil, fmt.Errorf("unrecognized event '%s'", tmp.Name)
> }
>
> it becomes feasible to stick with standard functions. We can of
> course keep the (Un)MarshalEvent functions around for convenience,
> but I don't think they should be the only available API.
I agree. I'll change it. Perhaps we shouldn't use
(Un)MarshalEvent at this layer at all. Probably the same for
(Un)MarshalCommand.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
2023-11-09 19:13 ` Victor Toso
@ 2023-11-10 9:52 ` Andrea Bolognani
0 siblings, 0 replies; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-10 9:52 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Thu, Nov 09, 2023 at 08:13:38PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> > Now, I'm not sure I would go as far as suggesting that the
> > GetName() function should be completely removed, but maybe we
> > can try leaving it out from the initial version and see if
> > people start screaming?
>
> It might be useful for debugging too. I would rather log
> e.GetName() than the string version of the type but if that's the
> only reason we needed, I agree on removing for now.
I think the upside is too small considering the potential for abuse.
> > API-wise, I'm not a fan of the fact that we're forcing users to call
> > (Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
> > something like
> >
> > func GetEventType(data []byte) (Event, error) {
> >
> > it becomes feasible to stick with standard functions. We can of
> > course keep the (Un)MarshalEvent functions around for convenience,
> > but I don't think they should be the only available API.
>
> I agree. I'll change it. Perhaps we shouldn't use
> (Un)MarshalEvent at this layer at all. Probably the same for
> (Un)MarshalCommand.
Yeah, what I wrote for events applies 1:1 to commands as well.
Up to you whether or not you want to keep around the convenience
functions. It might indeed be fine to drop them for now and consider
reintroducing them later if it turns out that it really helps making
client code less clunky.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 09/11] qapi: golang: Generate qapi's command types in Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (7 preceding siblings ...)
2023-10-16 15:27 ` [PATCH v2 08/11] qapi: golang: Generate qapi's event " Victor Toso
@ 2023-10-16 15:27 ` Victor Toso
2023-10-16 15:27 ` [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go Victor Toso
` (3 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch handles QAPI command types and generates data structures in
Go that decodes from QMP JSON Object to Go data structure and vice
versa.
Similar to Event, this patch adds a Command interface and two helper
functions MarshalCommand and UnmarshalCommand.
At the time of this writing, it generates 209 structures.
Example:
qapi:
| { 'command': 'set_password',
| 'boxed': true,
| 'data': 'SetPasswordOptions' }
go:
| type SetPasswordCommand struct {
| SetPasswordOptions
| MessageId string `json:"-"`
| }
usage:
| input := `{"execute":"set_password",` +
| `"arguments":{"protocol":"vnc",` +
| `"password":"secret"}}`
|
| c, err := UnmarshalCommand([]byte(input))
| if err != nil {
| panic(err)
| }
|
| if c.GetName() == `set_password` {
| m := c.(*SetPasswordCommand)
| // m.Password == "secret"
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 116 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 114 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 81b320d6dd..624bc2af4d 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -263,6 +263,51 @@
}}
"""
+TEMPLATE_COMMAND_METHODS = """
+func (c *{type_name}) GetName() string {{
+\treturn "{name}"
+}}
+
+func (s *{type_name}) GetId() string {{
+\treturn s.MessageId
+}}
+"""
+
+TEMPLATE_COMMAND = """
+type Command interface {{
+\tGetId() string
+\tGetName() string
+}}
+
+func MarshalCommand(c Command) ([]byte, error) {{
+\tm := make(map[string]any)
+\tm["execute"] = c.GetName()
+\tif id := c.GetId(); len(id) > 0 {{
+\t\tm["id"] = id
+\t}}
+\tif bytes, err := json.Marshal(c); err != nil {{
+\t\treturn []byte{{}}, err
+\t}} else if len(bytes) > 2 {{
+\t\tm["arguments"] = c
+\t}}
+\treturn json.Marshal(m)
+}}
+
+func UnmarshalCommand(data []byte) (Command, error) {{
+\tbase := struct {{
+\t\tMessageId string `json:"id,omitempty"`
+\t\tName string `json:"execute"`
+\t}}{{}}
+\tif err := json.Unmarshal(data, &base); err != nil {{
+\t\treturn nil, fmt.Errorf("Failed to decode command: %s", string(data))
+\t}}
+
+\tswitch base.Name {{{cases}
+\t}}
+\treturn nil, errors.New("Failed to recognize command")
+}}
+"""
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
@@ -301,7 +346,7 @@ def qapi_to_go_type_name(name: str, meta: Optional[str] = None) -> str:
name += "".join(word.title() for word in words[1:])
- types = ["event"]
+ types = ["event", "command"]
if meta in types:
name = name[:-3] if name.endswith("Arg") else name
name += meta.title().replace(" ", "")
@@ -670,6 +715,10 @@ def qapi_to_golang_struct(
"tag": """`json:"-"`""",
},
)
+ elif info.defn_meta == "command":
+ fields.insert(
+ 0, {"name": "MessageId", "type": "string", "tag": """`json:"-"`"""}
+ )
if members:
for member in members:
@@ -887,6 +936,28 @@ def generate_template_alternate(
return content
+def generate_template_command(commands: dict[str, Tuple[str, str]]) -> str:
+ cases = ""
+ content = ""
+ for name in sorted(commands):
+ case_type, gocode = commands[name]
+ content += gocode
+ cases += f"""
+case "{name}":
+ command := struct {{
+ Args {case_type} `json:"arguments"`
+ }}{{}}
+
+ if err := json.Unmarshal(data, &command); err != nil {{
+ return nil, fmt.Errorf("Failed to unmarshal: %s", string(data))
+ }}
+ command.Args.MessageId = base.MessageId
+ return &command.Args, nil
+"""
+ content += TEMPLATE_COMMAND.format(cases=cases)
+ return content
+
+
def generate_template_event(events: dict[str, Tuple[str, str]]) -> str:
cases = ""
content = ""
@@ -924,6 +995,7 @@ def __init__(self, _: str):
super().__init__()
types = (
"alternate",
+ "command",
"enum",
"event",
"helper",
@@ -933,6 +1005,7 @@ def __init__(self, _: str):
self.target = dict.fromkeys(types, "")
self.schema: QAPISchema
self.events: dict[str, Tuple[str, str]] = {}
+ self.commands: dict[str, Tuple[str, str]] = {}
self.golang_package_name = "qapi"
self.enums: dict[str, str] = {}
self.alternates: dict[str, str] = {}
@@ -988,6 +1061,7 @@ def visit_end(self) -> None:
self.target["struct"] += generate_content_from_dict(self.structs)
self.target["union"] += generate_content_from_dict(self.unions)
self.target["event"] += generate_template_event(self.events)
+ self.target["command"] += generate_template_command(self.commands)
def visit_object_type(
self,
@@ -1103,7 +1177,45 @@ def visit_command(
allow_preconfig: bool,
coroutine: bool,
) -> None:
- pass
+ assert name == info.defn_name
+ assert name not in self.commands
+
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+
+ content = ""
+ if boxed or not arg_type or not qapi_name_is_object(arg_type.name):
+ args: List[dict[str:str]] = []
+ if arg_type:
+ args.append(
+ {
+ "name": f"{arg_type.name}",
+ }
+ )
+ args.append(
+ {
+ "name": "MessageId",
+ "type": "string",
+ "tag": """`json:"-"`""",
+ }
+ )
+ content = generate_struct_type(type_name, args)
+ else:
+ assert isinstance(arg_type, QAPISchemaObjectType)
+ content = qapi_to_golang_struct(
+ self,
+ name,
+ arg_type.info,
+ arg_type.ifcond,
+ arg_type.features,
+ arg_type.base,
+ arg_type.members,
+ arg_type.variants,
+ )
+
+ content += TEMPLATE_COMMAND_METHODS.format(
+ name=name, type_name=type_name
+ )
+ self.commands[name] = (type_name, content)
def visit_event(
self,
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (8 preceding siblings ...)
2023-10-16 15:27 ` [PATCH v2 09/11] qapi: golang: Generate qapi's command " Victor Toso
@ 2023-10-16 15:27 ` Victor Toso
2023-11-09 18:24 ` Andrea Bolognani
2023-10-16 15:27 ` [PATCH v2 11/11] docs: add notes on Golang code generator Victor Toso
` (2 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
This patch adds a struct type in Go that will handle return values
for QAPI's command types.
The return value of a Command is, encouraged to be, QAPI's complex
types or an Array of those.
Every Command has a underlying CommandResult. The EmptyCommandReturn
is for those that don't expect any data e.g: `{ "return": {} }`.
All CommandReturn types implement the CommandResult interface.
Example:
qapi:
| { 'command': 'query-sev', 'returns': 'SevInfo',
| 'if': 'TARGET_I386' }
go:
| type QuerySevCommandReturn struct {
| MessageId string `json:"id,omitempty"`
| Result *SevInfo `json:"return"`
| Error *QapiError `json:"error,omitempty"`
| }
usage:
| // One can use QuerySevCommandReturn directly or
| // command's interface GetReturnType() instead.
|
| input := `{ "return": { "enabled": true, "api-major" : 0,` +
| `"api-minor" : 0, "build-id" : 0,` +
| `"policy" : 0, "state" : "running",` +
| `"handle" : 1 } } `
|
| ret := QuerySevCommandReturn{}
| err := json.Unmarshal([]byte(input), &ret)
| if ret.Error != nil {
| // Handle command failure {"error": { ...}}
| } else if ret.Result != nil {
| // ret.Result.Enable == true
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 104 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 92 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 624bc2af4d..6471ddeb52 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -39,6 +39,15 @@
"""
TEMPLATE_HELPER = """
+type QAPIError struct {
+\tClass string `json:"class"`
+\tDescription string `json:"desc"`
+}
+
+func (err *QAPIError) Error() string {
+\treturn err.Description
+}
+
// Creates a decoder that errors on unknown Fields
// Returns nil if successfully decoded @from payload to @into type
// Returns error if failed to decode @from payload to @into type
@@ -271,12 +280,17 @@
func (s *{type_name}) GetId() string {{
\treturn s.MessageId
}}
+
+func (s *{type_name}) GetReturnType() CommandReturn {{
+\treturn &{cmd_ret_name}{{}}
+}}
"""
TEMPLATE_COMMAND = """
type Command interface {{
\tGetId() string
\tGetName() string
+\tGetReturnType() CommandReturn
}}
func MarshalCommand(c Command) ([]byte, error) {{
@@ -308,6 +322,37 @@
}}
"""
+TEMPLATE_COMMAND_RETURN = """
+type CommandReturn interface {
+\tGetId() string
+\tGetCommandName() string
+\tGetError() error
+}
+"""
+
+TEMPLATE_COMMAND_RETURN_METHODS = """
+func (r *{cmd_ret_name}) GetCommandName() string {{
+\treturn "{name}"
+}}
+
+func (r *{cmd_ret_name}) GetId() string {{
+\treturn r.MessageId
+}}
+
+func (r *{cmd_ret_name}) GetError() error {{
+\treturn r.Error
+}}{marshal_empty}
+"""
+
+TEMPLATE_COMMAND_RETURN_MARSHAL_EMPTY = """
+func (r {cmd_ret_name}) MarshalJSON() ([]byte, error) {{
+\tif r.Error != nil {{
+\t\ttype Alias {cmd_ret_name}
+\t\treturn json.Marshal(Alias(r))
+\t}}
+\treturn []byte(`{{"return":{{}}}}`), nil
+}}"""
+
def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None:
vis = QAPISchemaGenGolangVisitor(prefix)
@@ -346,7 +391,7 @@ def qapi_to_go_type_name(name: str, meta: Optional[str] = None) -> str:
name += "".join(word.title() for word in words[1:])
- types = ["event", "command"]
+ types = ["event", "command", "command return"]
if meta in types:
name = name[:-3] if name.endswith("Arg") else name
name += meta.title().replace(" ", "")
@@ -943,18 +988,19 @@ def generate_template_command(commands: dict[str, Tuple[str, str]]) -> str:
case_type, gocode = commands[name]
content += gocode
cases += f"""
-case "{name}":
- command := struct {{
- Args {case_type} `json:"arguments"`
- }}{{}}
-
- if err := json.Unmarshal(data, &command); err != nil {{
- return nil, fmt.Errorf("Failed to unmarshal: %s", string(data))
- }}
- command.Args.MessageId = base.MessageId
- return &command.Args, nil
+\tcase "{name}":
+\t\tcommand := struct {{
+\t\t\tArgs {case_type} `json:"arguments"`
+\t\t}}{{}}
+
+\t\tif err := json.Unmarshal(data, &command); err != nil {{
+\t\t\treturn nil, fmt.Errorf("Failed to unmarshal: %s", string(data))
+\t\t}}
+\t\tcommand.Args.MessageId = base.MessageId
+\t\treturn &command.Args, nil
"""
content += TEMPLATE_COMMAND.format(cases=cases)
+ content += TEMPLATE_COMMAND_RETURN
return content
@@ -1182,6 +1228,34 @@ def visit_command(
type_name = qapi_to_go_type_name(name, info.defn_meta)
+ cmd_ret_name = qapi_to_go_type_name(name, "command return")
+ marshal_empty = TEMPLATE_COMMAND_RETURN_MARSHAL_EMPTY.format(
+ cmd_ret_name=cmd_ret_name
+ )
+ retargs: List[dict[str:str]] = [
+ {
+ "name": "MessageId",
+ "type": "string",
+ "tag": """`json:"id,omitempty"`""",
+ },
+ {
+ "name": "Error",
+ "type": "*QAPIError",
+ "tag": """`json:"error,omitempty"`""",
+ },
+ ]
+ if ret_type:
+ marshal_empty = ""
+ ret_type_name = qapi_schema_type_to_go_type(ret_type.name)
+ isptr = "*" if ret_type_name[0] not in "*[" else ""
+ retargs.append(
+ {
+ "name": "Result",
+ "type": f"{isptr}{ret_type_name}",
+ "tag": """`json:"return"`""",
+ }
+ )
+
content = ""
if boxed or not arg_type or not qapi_name_is_object(arg_type.name):
args: List[dict[str:str]] = []
@@ -1213,7 +1287,13 @@ def visit_command(
)
content += TEMPLATE_COMMAND_METHODS.format(
- name=name, type_name=type_name
+ name=name, type_name=type_name, cmd_ret_name=cmd_ret_name
+ )
+ content += generate_struct_type(cmd_ret_name, retargs)
+ content += TEMPLATE_COMMAND_RETURN_METHODS.format(
+ name=name,
+ cmd_ret_name=cmd_ret_name,
+ marshal_empty=marshal_empty,
)
self.commands[name] = (type_name, content)
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
2023-10-16 15:27 ` [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go Victor Toso
@ 2023-11-09 18:24 ` Andrea Bolognani
2023-11-09 19:31 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-09 18:24 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> This patch adds a struct type in Go that will handle return values
> for QAPI's command types.
>
> The return value of a Command is, encouraged to be, QAPI's complex
> types or an Array of those.
>
> Every Command has a underlying CommandResult. The EmptyCommandReturn
> is for those that don't expect any data e.g: `{ "return": {} }`.
>
> All CommandReturn types implement the CommandResult interface.
I guess EmptyCommandReturn is something that you have scrapped in
between revisions, because I see no reference to it in the current
incarnation of the code. Same thing with CommandResult, unless that's
just a typo for CommandReturn?
> Example:
> qapi:
> | { 'command': 'query-sev', 'returns': 'SevInfo',
> | 'if': 'TARGET_I386' }
>
> go:
> | type QuerySevCommandReturn struct {
> | MessageId string `json:"id,omitempty"`
> | Result *SevInfo `json:"return"`
> | Error *QapiError `json:"error,omitempty"`
> | }
>
> usage:
> | // One can use QuerySevCommandReturn directly or
> | // command's interface GetReturnType() instead.
I'm not convinced this function is particularly useful. I know that
I've suggested something similar for events, but the usage scenarios
are different.
For events, you're going to have some loop listening for them and you
can't know in advance what event you're going to receive at any given
time.
In contrast, a return value will be received as a direct consequence
of running a command, and since the caller knows what command it
called it's fair to assume that it also knows its return type.
> + if ret_type:
> + marshal_empty = ""
> + ret_type_name = qapi_schema_type_to_go_type(ret_type.name)
> + isptr = "*" if ret_type_name[0] not in "*[" else ""
> + retargs.append(
> + {
> + "name": "Result",
> + "type": f"{isptr}{ret_type_name}",
> + "tag": """`json:"return"`""",
> + }
> + )
This produces
type QueryAudiodevsCommandReturn struct {
MessageId string `json:"id,omitempty"`
Error *QAPIError `json:"error,omitempty"`
Result []Audiodev `json:"return"`
}
when the return type is an array. Is that the correct behavior? I
haven't thought too hard about it, but it seems odd so I though I'd
bring it up.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
2023-11-09 18:24 ` Andrea Bolognani
@ 2023-11-09 19:31 ` Victor Toso
2023-11-10 9:46 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-11-09 19:31 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 4866 bytes --]
Hi,
On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> > This patch adds a struct type in Go that will handle return values
> > for QAPI's command types.
> >
> > The return value of a Command is, encouraged to be, QAPI's complex
> > types or an Array of those.
> >
> > Every Command has a underlying CommandResult. The EmptyCommandReturn
> > is for those that don't expect any data e.g: `{ "return": {} }`.
> >
> > All CommandReturn types implement the CommandResult interface.
>
> I guess EmptyCommandReturn is something that you have scrapped
> in between revisions, because I see no reference to it in the
> current incarnation of the code. Same thing with CommandResult,
> unless that's just a typo for CommandReturn?
No typo. I did overlooked this commit log. We had
EmptyCommandReturn type in the past, we agreed to have a specific
type for each command even if they are empty (with basic/common
fields only)
>
> > Example:
> > qapi:
> > | { 'command': 'query-sev', 'returns': 'SevInfo',
> > | 'if': 'TARGET_I386' }
> >
> > go:
> > | type QuerySevCommandReturn struct {
> > | MessageId string `json:"id,omitempty"`
> > | Result *SevInfo `json:"return"`
> > | Error *QapiError `json:"error,omitempty"`
> > | }
> >
> > usage:
> > | // One can use QuerySevCommandReturn directly or
> > | // command's interface GetReturnType() instead.
>
> I'm not convinced this function is particularly useful. I know
> that I've suggested something similar for events, but the usage
> scenarios are different.
I think that I wanted to expose knowledge we had in the parser,
not necessarily useful or needed indeed. At the very least, I
agree that at this layer, we just want Command and ComandReturn
types to be generated and properly (un)mashalled.
One downside is for testing.
If we have a list of commands, I can just iterate over them
Unmarshal to a command interface variable, fetch the return type
and do some comparisons to see if all is what we expected. See:
https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61
Not saying we should keep it for tests, but it is useful :)
> For events, you're going to have some loop listening for them
> and you can't know in advance what event you're going to
> receive at any given time.
>
> In contrast, a return value will be received as a direct consequence
> of running a command, and since the caller knows what command it
> called it's fair to assume that it also knows its return type.
>
> > + if ret_type:
> > + marshal_empty = ""
> > + ret_type_name = qapi_schema_type_to_go_type(ret_type.name)
> > + isptr = "*" if ret_type_name[0] not in "*[" else ""
> > + retargs.append(
> > + {
> > + "name": "Result",
> > + "type": f"{isptr}{ret_type_name}",
> > + "tag": """`json:"return"`""",
> > + }
> > + )
>
> This produces
>
> type QueryAudiodevsCommandReturn struct {
> MessageId string `json:"id,omitempty"`
> Error *QAPIError `json:"error,omitempty"`
> Result []Audiodev `json:"return"`
> }
>
> when the return type is an array. Is that the correct behavior? I
> haven't thought too hard about it, but it seems odd so I though I'd
> bring it up.
Hm, the schema for it is
##
# @query-audiodevs:
#
# Returns information about audiodev configuration
#
# Returns: array of @Audiodev
#
# Since: 8.0
##
{ 'command': 'query-audiodevs',
'returns': ['Audiodev'] }
So, I think it is correct. Would you expect it to be an object
wrapping the array or I'm missing what you find odd.
# -> { "execute": "query-rx-filter", "arguments": { "name": "vnet0" } }
# <- { "return": [
# {
# "promiscuous": true,
# "name": "vnet0",
# "main-mac": "52:54:00:12:34:56",
# "unicast": "normal",
# "vlan": "normal",
# "vlan-table": [
# 4,
# 0
# ],
# "unicast-table": [
# ],
# "multicast": "normal",
# "multicast-overflow": false,
# "unicast-overflow": false,
# "multicast-table": [
# "01:00:5e:00:00:01",
# "33:33:00:00:00:01",
# "33:33:ff:12:34:56"
# ],
# "broadcast-allowed": false
# }
# ]
# }
##
{ 'command': 'query-rx-filter',
'data': { '*name': 'str' },
'returns': ['RxFilterInfo'] }
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
2023-11-09 19:31 ` Victor Toso
@ 2023-11-10 9:46 ` Andrea Bolognani
0 siblings, 0 replies; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-10 9:46 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Thu, Nov 09, 2023 at 08:31:01PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote:
> > On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> > > Example:
> > > qapi:
> > > | { 'command': 'query-sev', 'returns': 'SevInfo',
> > > | 'if': 'TARGET_I386' }
> > >
> > > go:
> > > | type QuerySevCommandReturn struct {
> > > | MessageId string `json:"id,omitempty"`
> > > | Result *SevInfo `json:"return"`
> > > | Error *QapiError `json:"error,omitempty"`
> > > | }
> > >
> > > usage:
> > > | // One can use QuerySevCommandReturn directly or
> > > | // command's interface GetReturnType() instead.
> >
> > I'm not convinced this function is particularly useful. I know
> > that I've suggested something similar for events, but the usage
> > scenarios are different.
>
> I think that I wanted to expose knowledge we had in the parser,
> not necessarily useful or needed indeed. At the very least, I
> agree that at this layer, we just want Command and ComandReturn
> types to be generated and properly (un)mashalled.
>
> One downside is for testing.
>
> If we have a list of commands, I can just iterate over them
> Unmarshal to a command interface variable, fetch the return type
> and do some comparisons to see if all is what we expected. See:
>
> https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61
>
> Not saying we should keep it for tests, but it is useful :)
That code is quite dense and I'm not going to dig into it now :)
Anyway, I don't have a problem with keeping this type-safe
introspection feature around. Maybe call it GetCommandReturnType
though.
> > This produces
> >
> > type QueryAudiodevsCommandReturn struct {
> > MessageId string `json:"id,omitempty"`
> > Error *QAPIError `json:"error,omitempty"`
> > Result []Audiodev `json:"return"`
> > }
> >
> > when the return type is an array. Is that the correct behavior? I
> > haven't thought too hard about it, but it seems odd so I though I'd
> > bring it up.
>
> Hm, the schema for it is
>
> ##
> # @query-audiodevs:
> #
> # Returns information about audiodev configuration
> #
> # Returns: array of @Audiodev
> #
> # Since: 8.0
> ##
> { 'command': 'query-audiodevs',
> 'returns': ['Audiodev'] }
>
> So, I think it is correct. Would you expect it to be an object
> wrapping the array or I'm missing what you find odd.
It works as expected if there is a result, but in the case of error:
in := `{ "error": {"class": "errorClass", "desc": "errorDesc" }}`
out := qapi.QueryAudiodevsCommandReturn{}
err := json.Unmarshal([]byte(in), &out)
if err != nil {
panic(err)
}
fmt.Printf("result=%v error=%v\n", out.Result, out.Error)
the output will be
result=[] error=errorDesc
Note how result is an empty array instead of a nil pointer. If we
unmarshal the same JSON into QueryReplayCommandReturn instead, the
output becomes
result=<nil> error=errorDesc
The latter behavior seems more correct to me, based on how we deal
with unions and alternates.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 11/11] docs: add notes on Golang code generator
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (9 preceding siblings ...)
2023-10-16 15:27 ` [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go Victor Toso
@ 2023-10-16 15:27 ` Victor Toso
2023-10-18 11:47 ` Markus Armbruster
2023-10-27 17:33 ` [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
2023-11-09 18:35 ` Andrea Bolognani
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-16 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
The goal of this patch is converge discussions into a documentation,
to make it easy and explicit design decisions, known issues and what
else might help a person interested in how the Go module is generated.
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
docs/devel/index-build.rst | 1 +
docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
2 files changed, 377 insertions(+)
create mode 100644 docs/devel/qapi-golang-code-gen.rst
diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
index 57e8d39d98..8f7c6f5dc7 100644
--- a/docs/devel/index-build.rst
+++ b/docs/devel/index-build.rst
@@ -15,5 +15,6 @@ the basics if you are adding new files and targets to the build.
qtest
ci
qapi-code-gen
+ qapi-golang-code-gen
fuzzing
control-flow-integrity
diff --git a/docs/devel/qapi-golang-code-gen.rst b/docs/devel/qapi-golang-code-gen.rst
new file mode 100644
index 0000000000..b62daf3bad
--- /dev/null
+++ b/docs/devel/qapi-golang-code-gen.rst
@@ -0,0 +1,376 @@
+==========================
+QAPI Golang code generator
+==========================
+
+..
+ Copyright (C) 2023 Red Hat, Inc.
+
+ This work is licensed under the terms of the GNU GPL, version 2 or
+ later. See the COPYING file in the top-level directory.
+
+
+Introduction
+============
+
+This document provides information of how the generated Go code maps
+with the QAPI specification, clarifying design decisions when needed.
+
+
+Scope of the generated Go code
+==============================
+
+The scope is limited to data structures that can interpret and be used
+to generate valid QMP messages. These data structures are generated
+from a QAPI schema and should be able to handle QMP messages from the
+same schema.
+
+The generated Go code is a Go module with data structs that uses Go
+standard library ``encoding/json``, implementing its field tags and
+Marshal interface whenever needed.
+
+
+QAPI types to Go structs
+========================
+
+Enum
+----
+
+Enums are mapped as strings in Go, using a specified string type per
+Enum to help with type safety in the Go application.
+
+::
+
+ { 'enum': 'HostMemPolicy',
+ 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
+
+.. code-block:: go
+
+ type HostMemPolicy string
+
+ const (
+ HostMemPolicyDefault HostMemPolicy = "default"
+ HostMemPolicyPreferred HostMemPolicy = "preferred"
+ HostMemPolicyBind HostMemPolicy = "bind"
+ HostMemPolicyInterleave HostMemPolicy = "interleave"
+ )
+
+
+Struct
+------
+
+The mapping between a QAPI struct in Go struct is very straightforward.
+ - Each member of the QAPI struct has its own field in a Go struct.
+ - Optional members are pointers type with 'omitempty' field tag set
+
+One important design decision was to _not_ embed base struct, copying
+the base members to the original struct. This reduces the complexity
+for the Go application.
+
+::
+
+ { 'struct': 'BlockExportOptionsNbdBase',
+ 'data': { '*name': 'str', '*description': 'str' } }
+
+ { 'struct': 'BlockExportOptionsNbd',
+ 'base': 'BlockExportOptionsNbdBase',
+ 'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
+ '*allocation-depth': 'bool' } }
+
+.. code-block:: go
+
+ type BlockExportOptionsNbd struct {
+ Name *string `json:"name,omitempty"`
+ Description *string `json:"description,omitempty"`
+
+ Bitmaps []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
+ AllocationDepth *bool `json:"allocation-depth,omitempty"`
+ }
+
+
+Union
+-----
+
+Unions in QAPI are binded to a Enum type which provides all possible
+branches of the union. The most important caveat here is that the Union
+does not need to have a complex type implemented for all possible
+branches of the Enum. Receiving a enum value of a unimplemented branch
+is valid.
+
+For this reason, the generated Go struct will define a field for each
+Enum value. The Go type defined for unbranched Enum values is bool
+
+Go struct and also implement the Marshal interface.
+
+As each Union Go struct type has both the discriminator field and
+optional fields, it is important to note that when converting Go struct
+to JSON, we only consider the discriminator field if no optional field
+member was set. In practice, the user should use the optional fields if
+the QAPI Union type has defined them, otherwise the user can set the
+discriminator field for the unbranched enum value.
+
+::
+
+ { 'union': 'ImageInfoSpecificQCow2Encryption',
+ 'base': 'ImageInfoSpecificQCow2EncryptionBase',
+ 'discriminator': 'format',
+ 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
+
+.. code-block:: go
+
+ type ImageInfoSpecificQCow2Encryption struct {
+ // Variants fields
+ Luks *QCryptoBlockInfoLUKS `json:"-"`
+ // Unbranched enum fields
+ Aes bool `json:"-"`
+ }
+
+ func (s ImageInfoSpecificQCow2Encryption) MarshalJSON() ([]byte, error) {
+ // ...
+ // Logic for branched Enum
+ if s.Luks != nil && err == nil {
+ if len(bytes) != 0 {
+ err = errors.New(`multiple variant fields set`)
+ } else if err = unwrapToMap(m, s.Luks); err == nil {
+ m["format"] = BlockdevQcow2EncryptionFormatLuks
+ bytes, err = json.Marshal(m)
+ }
+ }
+
+ // Logic for unbranched Enum
+ if s.Aes && err == nil {
+ if len(bytes) != 0 {
+ err = errors.New(`multiple variant fields set`)
+ } else {
+ m["format"] = BlockdevQcow2EncryptionFormatAes
+ bytes, err = json.Marshal(m)
+ }
+ }
+
+ // ...
+ // Handle errors
+ }
+
+
+ func (s *ImageInfoSpecificQCow2Encryption) UnmarshalJSON(data []byte) error {
+ // ...
+
+ switch tmp.Format {
+ case BlockdevQcow2EncryptionFormatLuks:
+ s.Luks = new(QCryptoBlockInfoLUKS)
+ if err := json.Unmarshal(data, s.Luks); err != nil {
+ s.Luks = nil
+ return err
+ }
+ case BlockdevQcow2EncryptionFormatAes:
+ s.Aes = true
+
+ default:
+ return fmt.Errorf("error: unmarshal: ImageInfoSpecificQCow2Encryption: received unrecognized value: '%s'",
+ tmp.Format)
+ }
+ return nil
+ }
+
+
+Alternate
+---------
+
+Like Unions, alternates can have a few branches. Unlike Unions, they
+don't have a discriminator field and each branch should be a different
+class of Type entirely (e.g: You can't have two branches of type int in
+one Alternate).
+
+While the marshalling is similar to Unions, the unmarshalling uses a
+try-and-error approach, trying to fit the data payload in one of the
+Alternate fields.
+
+The biggest caveat is handling Alternates that can take JSON Null as
+value. The issue lies on ``encoding/json`` library limitation where
+unmarshalling JSON Null data to a Go struct which has the 'omitempty'
+field that, it bypass the Marshal interface. The same happens when
+marshalling, if the field tag 'omitempty' is used, a nil pointer would
+never be translated to null JSON value.
+
+The problem being, we use pointer to type plus ``omitempty`` field to
+express a QAPI optional member.
+
+In order to handle JSON Null, the generator needs to do the following:
+ - Read the QAPI schema prior to generate any code and cache
+ all alternate types that can take JSON Null
+ - For all Go structs that should be considered optional and they type
+ are one of those alternates, do not set ``omitempty`` and implement
+ Marshal interface for this Go struct, to properly handle JSON Null
+ - In the Alternate, uses a boolean 'IsNull' to express a JSON Null
+ and implement the AbsentAlternate interface, to help sturcts know
+ if a given Alternate type should be considered Absent (not set) or
+ any other possible Value, including JSON Null.
+
+::
+
+ { 'alternate': 'BlockdevRefOrNull',
+ 'data': { 'definition': 'BlockdevOptions',
+ 'reference': 'str',
+ 'null': 'null' } }
+
+.. code-block:: go
+
+ type BlockdevRefOrNull struct {
+ Definition *BlockdevOptions
+ Reference *string
+ IsNull bool
+ }
+
+ func (s *BlockdevRefOrNull) ToAnyOrAbsent() (any, bool) {
+ if s != nil {
+ if s.IsNull {
+ return nil, false
+ } else if s.Definition != nil {
+ return *s.Definition, false
+ } else if s.Reference != nil {
+ return *s.Reference, false
+ }
+ }
+
+ return nil, true
+ }
+
+ func (s BlockdevRefOrNull) MarshalJSON() ([]byte, error) {
+ if s.IsNull {
+ return []byte("null"), nil
+ } else if s.Definition != nil {
+ return json.Marshal(s.Definition)
+ } else if s.Reference != nil {
+ return json.Marshal(s.Reference)
+ }
+ return []byte("{}"), nil
+ }
+
+ func (s *BlockdevRefOrNull) UnmarshalJSON(data []byte) error {
+ // Check for json-null first
+ if string(data) == "null" {
+ s.IsNull = true
+ return nil
+ }
+ // Check for BlockdevOptions
+ {
+ s.Definition = new(BlockdevOptions)
+ if err := StrictDecode(s.Definition, data); err == nil {
+ return nil
+ }
+ s.Definition = nil
+ }
+ // Check for string
+ {
+ s.Reference = new(string)
+ if err := StrictDecode(s.Reference, data); err == nil {
+ return nil
+ }
+ s.Reference = nil
+ }
+
+ return fmt.Errorf("Can't convert to BlockdevRefOrNull: %s", string(data))
+ }
+
+
+Event
+-----
+
+All events are mapped to its own struct with the additional
+MessageTimestamp field, for the over-the-wire 'timestamp' value.
+
+Marshaling and Unmarshaling happens over the Event interface, so users
+should use the MarshalEvent() and UnmarshalEvent() methods.
+
+::
+
+ { 'event': 'SHUTDOWN',
+ 'data': { 'guest': 'bool',
+ 'reason': 'ShutdownCause' } }
+
+.. code-block:: go
+
+ type Event interface {
+ GetName() string
+ GetTimestamp() Timestamp
+ }
+
+ type ShutdownEvent struct {
+ MessageTimestamp Timestamp `json:"-"`
+ Guest bool `json:"guest"`
+ Reason ShutdownCause `json:"reason"`
+ }
+
+ func (s *ShutdownEvent) GetName() string {
+ return "SHUTDOWN"
+ }
+
+ func (s *ShutdownEvent) GetTimestamp() Timestamp {
+ return s.MessageTimestamp
+ }
+
+
+Command
+-------
+
+All commands are mapped to its own struct with the additional MessageId
+field for the optional 'id'. If the command has a boxed data struct,
+the option struct will be embed in the command struct.
+
+As commands do require a return value, every command has its own return
+type. The Command interface has a GetReturnType() method that returns a
+CommandReturn interface, to help Go application handling the data.
+
+Marshaling and Unmarshaling happens over the Command interface, so
+users should use the MarshalCommand() and UnmarshalCommand() methods.
+
+::
+
+ { 'command': 'set_password',
+ 'boxed': true,
+ 'data': 'SetPasswordOptions' }
+
+.. code-block:: go
+
+ type Command interface {
+ GetId() string
+ GetName() string
+ GetReturnType() CommandReturn
+ }
+
+ // SetPasswordOptions is embed
+ type SetPasswordCommand struct {
+ SetPasswordOptions
+ MessageId string `json:"-"`
+ }
+
+ // This is an union
+ type SetPasswordOptions struct {
+ Protocol DisplayProtocol `json:"protocol"`
+ Password string `json:"password"`
+ Connected *SetPasswordAction `json:"connected,omitempty"`
+
+ // Variants fields
+ Vnc *SetPasswordOptionsVnc `json:"-"`
+ }
+
+Now an example of a command without boxed type.
+
+::
+
+ { 'command': 'set_link',
+ 'data': {'name': 'str', 'up': 'bool'} }
+
+.. code-block:: go
+
+ type SetLinkCommand struct {
+ MessageId string `json:"-"`
+ Name string `json:"name"`
+ Up bool `json:"up"`
+ }
+
+Known issues
+============
+
+- Type names might not follow proper Go convention. Andrea suggested an
+ annotation to the QAPI schema that could solve it.
+ https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg00127.html
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/11] docs: add notes on Golang code generator
2023-10-16 15:27 ` [PATCH v2 11/11] docs: add notes on Golang code generator Victor Toso
@ 2023-10-18 11:47 ` Markus Armbruster
2023-10-18 16:21 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Markus Armbruster @ 2023-10-18 11:47 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
Victor Toso <victortoso@redhat.com> writes:
> The goal of this patch is converge discussions into a documentation,
> to make it easy and explicit design decisions, known issues and what
> else might help a person interested in how the Go module is generated.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
> docs/devel/index-build.rst | 1 +
> docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
> 2 files changed, 377 insertions(+)
> create mode 100644 docs/devel/qapi-golang-code-gen.rst
>
> diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
> index 57e8d39d98..8f7c6f5dc7 100644
> --- a/docs/devel/index-build.rst
> +++ b/docs/devel/index-build.rst
> @@ -15,5 +15,6 @@ the basics if you are adding new files and targets to the build.
> qtest
> ci
> qapi-code-gen
> + qapi-golang-code-gen
> fuzzing
> control-flow-integrity
Let's not worry whether and how this should be integrated with
qapi-code-gen.rst for now.
I'm a Go ignoramus. I hope my comments are at least somewhat helpful
all the same.
> diff --git a/docs/devel/qapi-golang-code-gen.rst b/docs/devel/qapi-golang-code-gen.rst
> new file mode 100644
> index 0000000000..b62daf3bad
> --- /dev/null
> +++ b/docs/devel/qapi-golang-code-gen.rst
> @@ -0,0 +1,376 @@
> +==========================
> +QAPI Golang code generator
> +==========================
> +
> +..
> + Copyright (C) 2023 Red Hat, Inc.
> +
> + This work is licensed under the terms of the GNU GPL, version 2 or
> + later. See the COPYING file in the top-level directory.
> +
> +
> +Introduction
> +============
> +
> +This document provides information of how the generated Go code maps
> +with the QAPI specification, clarifying design decisions when needed.
> +
> +
> +Scope of the generated Go code
> +==============================
What do you mean by "scope"?
> +
> +The scope is limited to data structures that can interpret and be used
> +to generate valid QMP messages. These data structures are generated
> +from a QAPI schema and should be able to handle QMP messages from the
> +same schema.
> +
> +The generated Go code is a Go module with data structs that uses Go
> +standard library ``encoding/json``, implementing its field tags and
> +Marshal interface whenever needed.
> +
> +
> +QAPI types to Go structs
> +========================
> +
> +Enum
> +----
> +
> +Enums are mapped as strings in Go, using a specified string type per
> +Enum to help with type safety in the Go application.
> +
> +::
> +
> + { 'enum': 'HostMemPolicy',
> + 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
> +
> +.. code-block:: go
> +
> + type HostMemPolicy string
> +
> + const (
> + HostMemPolicyDefault HostMemPolicy = "default"
> + HostMemPolicyPreferred HostMemPolicy = "preferred"
> + HostMemPolicyBind HostMemPolicy = "bind"
> + HostMemPolicyInterleave HostMemPolicy = "interleave"
> + )
> +
> +
> +Struct
> +------
> +
> +The mapping between a QAPI struct in Go struct is very straightforward.
> + - Each member of the QAPI struct has its own field in a Go struct.
> + - Optional members are pointers type with 'omitempty' field tag set
> +
> +One important design decision was to _not_ embed base struct, copying
> +the base members to the original struct. This reduces the complexity
> +for the Go application.
> +
> +::
> +
> + { 'struct': 'BlockExportOptionsNbdBase',
> + 'data': { '*name': 'str', '*description': 'str' } }
> +
> + { 'struct': 'BlockExportOptionsNbd',
> + 'base': 'BlockExportOptionsNbdBase',
> + 'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
> + '*allocation-depth': 'bool' } }
> +
> +.. code-block:: go
> +
> + type BlockExportOptionsNbd struct {
> + Name *string `json:"name,omitempty"`
> + Description *string `json:"description,omitempty"`
> +
> + Bitmaps []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
> + AllocationDepth *bool `json:"allocation-depth,omitempty"`
> + }
Is there a need to convert from type to base type?
In C, we get away with (BlockExportOptionsNbdBase *)obj. We hide it in
an inline function, though.
> +
> +
> +Union
> +-----
> +
> +Unions in QAPI are binded to a Enum type which provides all possible
bound to an Enum
> +branches of the union. The most important caveat here is that the Union
> +does not need to have a complex type implemented for all possible
> +branches of the Enum. Receiving a enum value of a unimplemented branch
I'd call that branch empty, not unimplemented.
> +is valid.
> +
> +For this reason, the generated Go struct will define a field for each
> +Enum value. The Go type defined for unbranched Enum values is bool
Important design decision: since Go sucks at sum types, you elect to add
all the variant members to the struct. Only one of them may be used at
any time. Please spell that out more clearly.
> +
> +Go struct and also implement the Marshal interface.
Blank line in the middle of a sentence? Or two sentences? Can't make
sense of them.
What do you mean by "unbranched" Enum value? Enum value with an
implicit (empty) branch?
> +
> +As each Union Go struct type has both the discriminator field and
> +optional fields, it is important to note that when converting Go struct
> +to JSON, we only consider the discriminator field if no optional field
> +member was set. In practice, the user should use the optional fields if
> +the QAPI Union type has defined them, otherwise the user can set the
> +discriminator field for the unbranched enum value.
I don't think I get this paragraph.
> +
> +::
> +
> + { 'union': 'ImageInfoSpecificQCow2Encryption',
> + 'base': 'ImageInfoSpecificQCow2EncryptionBase',
> + 'discriminator': 'format',
> + 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
The example is hard to understand without additional context, namely:
{ 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
{ 'enum': 'BlockdevQcow2EncryptionFormat',
'data': [ 'aes', 'luks' ] }
> +
> +.. code-block:: go
> +
> + type ImageInfoSpecificQCow2Encryption struct {
> + // Variants fields
> + Luks *QCryptoBlockInfoLUKS `json:"-"`
> + // Unbranched enum fields
> + Aes bool `json:"-"`
> + }
The members of the base type are the common members, and the members of
the branch's type are the variant members.
Your example shows the variant member: 'luks'.
The common member @format isn't there. I guess you're eliding it
because you can derive its value from other members.
If there were other common members, where would they go?
> +
> + func (s ImageInfoSpecificQCow2Encryption) MarshalJSON() ([]byte, error) {
> + // ...
> + // Logic for branched Enum
> + if s.Luks != nil && err == nil {
> + if len(bytes) != 0 {
> + err = errors.New(`multiple variant fields set`)
> + } else if err = unwrapToMap(m, s.Luks); err == nil {
> + m["format"] = BlockdevQcow2EncryptionFormatLuks
> + bytes, err = json.Marshal(m)
> + }
> + }
> +
> + // Logic for unbranched Enum
> + if s.Aes && err == nil {
> + if len(bytes) != 0 {
> + err = errors.New(`multiple variant fields set`)
> + } else {
> + m["format"] = BlockdevQcow2EncryptionFormatAes
> + bytes, err = json.Marshal(m)
> + }
> + }
> +
> + // ...
> + // Handle errors
> + }
> +
> +
> + func (s *ImageInfoSpecificQCow2Encryption) UnmarshalJSON(data []byte) error {
> + // ...
> +
> + switch tmp.Format {
> + case BlockdevQcow2EncryptionFormatLuks:
> + s.Luks = new(QCryptoBlockInfoLUKS)
> + if err := json.Unmarshal(data, s.Luks); err != nil {
> + s.Luks = nil
> + return err
> + }
> + case BlockdevQcow2EncryptionFormatAes:
> + s.Aes = true
> +
> + default:
> + return fmt.Errorf("error: unmarshal: ImageInfoSpecificQCow2Encryption: received unrecognized value: '%s'",
> + tmp.Format)
> + }
> + return nil
> + }
> +
> +
> +Alternate
> +---------
> +
> +Like Unions, alternates can have a few branches. Unlike Unions, they
Scratch "a few".
> +don't have a discriminator field and each branch should be a different
> +class of Type entirely (e.g: You can't have two branches of type int in
> +one Alternate).
> +
> +While the marshalling is similar to Unions, the unmarshalling uses a
> +try-and-error approach, trying to fit the data payload in one of the
> +Alternate fields.
> +
> +The biggest caveat is handling Alternates that can take JSON Null as
> +value. The issue lies on ``encoding/json`` library limitation where
> +unmarshalling JSON Null data to a Go struct which has the 'omitempty'
> +field that, it bypass the Marshal interface. The same happens when
> +marshalling, if the field tag 'omitempty' is used, a nil pointer would
> +never be translated to null JSON value.
> +
> +The problem being, we use pointer to type plus ``omitempty`` field to
> +express a QAPI optional member.
> +
> +In order to handle JSON Null, the generator needs to do the following:
> + - Read the QAPI schema prior to generate any code and cache
> + all alternate types that can take JSON Null
> + - For all Go structs that should be considered optional and they type
> + are one of those alternates, do not set ``omitempty`` and implement
> + Marshal interface for this Go struct, to properly handle JSON Null
> + - In the Alternate, uses a boolean 'IsNull' to express a JSON Null
> + and implement the AbsentAlternate interface, to help sturcts know
Typo: to help structs
> + if a given Alternate type should be considered Absent (not set) or
> + any other possible Value, including JSON Null.
> +
> +::
> +
> + { 'alternate': 'BlockdevRefOrNull',
> + 'data': { 'definition': 'BlockdevOptions',
> + 'reference': 'str',
> + 'null': 'null' } }
> +
> +.. code-block:: go
> +
> + type BlockdevRefOrNull struct {
> + Definition *BlockdevOptions
> + Reference *string
> + IsNull bool
> + }
> +
> + func (s *BlockdevRefOrNull) ToAnyOrAbsent() (any, bool) {
> + if s != nil {
> + if s.IsNull {
> + return nil, false
> + } else if s.Definition != nil {
> + return *s.Definition, false
> + } else if s.Reference != nil {
> + return *s.Reference, false
> + }
> + }
> +
> + return nil, true
> + }
> +
> + func (s BlockdevRefOrNull) MarshalJSON() ([]byte, error) {
> + if s.IsNull {
> + return []byte("null"), nil
> + } else if s.Definition != nil {
> + return json.Marshal(s.Definition)
> + } else if s.Reference != nil {
> + return json.Marshal(s.Reference)
> + }
> + return []byte("{}"), nil
> + }
> +
> + func (s *BlockdevRefOrNull) UnmarshalJSON(data []byte) error {
> + // Check for json-null first
> + if string(data) == "null" {
> + s.IsNull = true
> + return nil
> + }
> + // Check for BlockdevOptions
> + {
> + s.Definition = new(BlockdevOptions)
> + if err := StrictDecode(s.Definition, data); err == nil {
> + return nil
> + }
> + s.Definition = nil
> + }
> + // Check for string
> + {
> + s.Reference = new(string)
> + if err := StrictDecode(s.Reference, data); err == nil {
> + return nil
> + }
> + s.Reference = nil
> + }
> +
> + return fmt.Errorf("Can't convert to BlockdevRefOrNull: %s", string(data))
> + }
> +
> +
> +Event
> +-----
> +
> +All events are mapped to its own struct with the additional
Each event is mapped to its own
> +MessageTimestamp field, for the over-the-wire 'timestamp' value.
> +
> +Marshaling and Unmarshaling happens over the Event interface, so users
> +should use the MarshalEvent() and UnmarshalEvent() methods.
> +
> +::
> +
> + { 'event': 'SHUTDOWN',
> + 'data': { 'guest': 'bool',
> + 'reason': 'ShutdownCause' } }
> +
> +.. code-block:: go
> +
> + type Event interface {
> + GetName() string
> + GetTimestamp() Timestamp
> + }
> +
> + type ShutdownEvent struct {
> + MessageTimestamp Timestamp `json:"-"`
> + Guest bool `json:"guest"`
> + Reason ShutdownCause `json:"reason"`
> + }
> +
> + func (s *ShutdownEvent) GetName() string {
> + return "SHUTDOWN"
> + }
> +
> + func (s *ShutdownEvent) GetTimestamp() Timestamp {
> + return s.MessageTimestamp
> + }
> +
> +
> +Command
> +-------
> +
> +All commands are mapped to its own struct with the additional MessageId
Each command is mapped to its own
> +field for the optional 'id'. If the command has a boxed data struct,
> +the option struct will be embed in the command struct.
> +
> +As commands do require a return value, every command has its own return
> +type. The Command interface has a GetReturnType() method that returns a
> +CommandReturn interface, to help Go application handling the data.
> +
> +Marshaling and Unmarshaling happens over the Command interface, so
> +users should use the MarshalCommand() and UnmarshalCommand() methods.
> +
> +::
> +
> + { 'command': 'set_password',
> + 'boxed': true,
> + 'data': 'SetPasswordOptions' }
Since you show the Go type generated for QAPI type SetPasswordOptions,
you should show the QAPI type here.
> +
> +.. code-block:: go
> +
> + type Command interface {
> + GetId() string
> + GetName() string
> + GetReturnType() CommandReturn
> + }
> +
> + // SetPasswordOptions is embed
> + type SetPasswordCommand struct {
> + SetPasswordOptions
> + MessageId string `json:"-"`
> + }
> +
> + // This is an union
> + type SetPasswordOptions struct {
> + Protocol DisplayProtocol `json:"protocol"`
> + Password string `json:"password"`
> + Connected *SetPasswordAction `json:"connected,omitempty"`
> +
> + // Variants fields
> + Vnc *SetPasswordOptionsVnc `json:"-"`
> + }
> +
> +Now an example of a command without boxed type.
> +
> +::
> +
> + { 'command': 'set_link',
> + 'data': {'name': 'str', 'up': 'bool'} }
> +
> +.. code-block:: go
> +
> + type SetLinkCommand struct {
> + MessageId string `json:"-"`
> + Name string `json:"name"`
> + Up bool `json:"up"`
> + }
> +
> +Known issues
> +============
> +
> +- Type names might not follow proper Go convention. Andrea suggested an
> + annotation to the QAPI schema that could solve it.
> + https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg00127.html
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/11] docs: add notes on Golang code generator
2023-10-18 11:47 ` Markus Armbruster
@ 2023-10-18 16:21 ` Victor Toso
2023-10-19 6:56 ` Markus Armbruster
0 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-18 16:21 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
[-- Attachment #1: Type: text/plain, Size: 18745 bytes --]
Hi,
On Wed, Oct 18, 2023 at 01:47:56PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > The goal of this patch is converge discussions into a documentation,
> > to make it easy and explicit design decisions, known issues and what
> > else might help a person interested in how the Go module is generated.
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> > docs/devel/index-build.rst | 1 +
> > docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
> > 2 files changed, 377 insertions(+)
> > create mode 100644 docs/devel/qapi-golang-code-gen.rst
> >
> > diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
> > index 57e8d39d98..8f7c6f5dc7 100644
> > --- a/docs/devel/index-build.rst
> > +++ b/docs/devel/index-build.rst
> > @@ -15,5 +15,6 @@ the basics if you are adding new files and targets to the build.
> > qtest
> > ci
> > qapi-code-gen
> > + qapi-golang-code-gen
> > fuzzing
> > control-flow-integrity
>
> Let's not worry whether and how this should be integrated with
> qapi-code-gen.rst for now.
>
> I'm a Go ignoramus. I hope my comments are at least somewhat
> helpful all the same.
They always are.
> > diff --git a/docs/devel/qapi-golang-code-gen.rst b/docs/devel/qapi-golang-code-gen.rst
> > new file mode 100644
> > index 0000000000..b62daf3bad
> > --- /dev/null
> > +++ b/docs/devel/qapi-golang-code-gen.rst
> > @@ -0,0 +1,376 @@
> > +==========================
> > +QAPI Golang code generator
> > +==========================
> > +
> > +..
> > + Copyright (C) 2023 Red Hat, Inc.
> > +
> > + This work is licensed under the terms of the GNU GPL, version 2 or
> > + later. See the COPYING file in the top-level directory.
> > +
> > +
> > +Introduction
> > +============
> > +
> > +This document provides information of how the generated Go code maps
> > +with the QAPI specification, clarifying design decisions when needed.
> > +
> > +
> > +Scope of the generated Go code
> > +==============================
>
> What do you mean by "scope"?
To build an application to talk with QEMU over QMP, this
generated code is not enough. What I mean is that, this is just
the first layer. We still need a library on top to do the work of
connecting, sending/receiving messages, etc.
Any recommendations on how to word this better?
> > +
> > +The scope is limited to data structures that can interpret and be used
> > +to generate valid QMP messages. These data structures are generated
> > +from a QAPI schema and should be able to handle QMP messages from the
> > +same schema.
> > +
> > +The generated Go code is a Go module with data structs that uses Go
> > +standard library ``encoding/json``, implementing its field tags and
> > +Marshal interface whenever needed.
> > +
> > +
> > +QAPI types to Go structs
> > +========================
> > +
> > +Enum
> > +----
> > +
> > +Enums are mapped as strings in Go, using a specified string type per
> > +Enum to help with type safety in the Go application.
> > +
> > +::
> > +
> > + { 'enum': 'HostMemPolicy',
> > + 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
> > +
> > +.. code-block:: go
> > +
> > + type HostMemPolicy string
> > +
> > + const (
> > + HostMemPolicyDefault HostMemPolicy = "default"
> > + HostMemPolicyPreferred HostMemPolicy = "preferred"
> > + HostMemPolicyBind HostMemPolicy = "bind"
> > + HostMemPolicyInterleave HostMemPolicy = "interleave"
> > + )
> > +
> > +
> > +Struct
> > +------
> > +
> > +The mapping between a QAPI struct in Go struct is very straightforward.
> > + - Each member of the QAPI struct has its own field in a Go struct.
> > + - Optional members are pointers type with 'omitempty' field tag set
> > +
> > +One important design decision was to _not_ embed base struct, copying
> > +the base members to the original struct. This reduces the complexity
> > +for the Go application.
> > +
> > +::
> > +
> > + { 'struct': 'BlockExportOptionsNbdBase',
> > + 'data': { '*name': 'str', '*description': 'str' } }
> > +
> > + { 'struct': 'BlockExportOptionsNbd',
> > + 'base': 'BlockExportOptionsNbdBase',
> > + 'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
> > + '*allocation-depth': 'bool' } }
> > +
> > +.. code-block:: go
> > +
> > + type BlockExportOptionsNbd struct {
> > + Name *string `json:"name,omitempty"`
> > + Description *string `json:"description,omitempty"`
> > +
> > + Bitmaps []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
> > + AllocationDepth *bool `json:"allocation-depth,omitempty"`
> > + }
>
> Is there a need to convert from type to base type?
Do you mean why we don't embed a base struct and do a copy of its
fields instead? If yes, the main reason is aesthetic. See this
issue: https://github.com/golang/go/issues/29438
So, we could have a situation where we have a embed type, inside
a embed type, inside of embed type making it horrible to write
the code for using it.
> In C, we get away with (BlockExportOptionsNbdBase *)obj. We hide it in
> an inline function, though.
>
> > +
> > +
> > +Union
> > +-----
> > +
> > +Unions in QAPI are binded to a Enum type which provides all possible
>
> bound to an Enum
Fixed.
> > +branches of the union. The most important caveat here is that the Union
> > +does not need to have a complex type implemented for all possible
> > +branches of the Enum. Receiving a enum value of a unimplemented branch
>
> I'd call that branch empty, not unimplemented.
Ok, changed them all to empty branch.
> > +is valid.
> > +
> > +For this reason, the generated Go struct will define a field for each
> > +Enum value. The Go type defined for unbranched Enum values is bool
>
> Important design decision: since Go sucks at sum types, you elect to add
> all the variant members to the struct. Only one of them may be used at
> any time. Please spell that out more clearly.
What about:
The generated Go struct will then define a field for each
Enum value. The type for Enum values of empty branch is bool.
Only one field can be set at time.
> > +
> > +Go struct and also implement the Marshal interface.
>
> Blank line in the middle of a sentence? Or two sentences? Can't make
> sense of them.
Leftover when rewriting it. Removed it.
> What do you mean by "unbranched" Enum value? Enum value with an
> implicit (empty) branch?
Yes, changing it to empty branch or empty branched.
> > +
> > +As each Union Go struct type has both the discriminator field and
> > +optional fields, it is important to note that when converting Go struct
> > +to JSON, we only consider the discriminator field if no optional field
> > +member was set. In practice, the user should use the optional fields if
> > +the QAPI Union type has defined them, otherwise the user can set the
> > +discriminator field for the unbranched enum value.
>
> I don't think I get this paragraph.
Sorry, leftover again. I've removed it entirely.
This bit was when we had a Discriminator field, we don't have it
anymore.
> > +
> > +::
> > +
> > + { 'union': 'ImageInfoSpecificQCow2Encryption',
> > + 'base': 'ImageInfoSpecificQCow2EncryptionBase',
> > + 'discriminator': 'format',
> > + 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
>
> The example is hard to understand without additional context, namely:
>
> { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
> 'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>
> { 'enum': 'BlockdevQcow2EncryptionFormat',
> 'data': [ 'aes', 'luks' ] }
Added.
> > +
> > +.. code-block:: go
> > +
> > + type ImageInfoSpecificQCow2Encryption struct {
> > + // Variants fields
> > + Luks *QCryptoBlockInfoLUKS `json:"-"`
> > + // Unbranched enum fields
> > + Aes bool `json:"-"`
> > + }
>
> The members of the base type are the common members, and the members of
> the branch's type are the variant members.
>
> Your example shows the variant member: 'luks'.
>
> The common member @format isn't there. I guess you're eliding it
> because you can derive its value from other members.
Correct. We can define @format value based on user's setting Aes
or Luks.
> If there were other common members, where would they go?
They should all be at the top of the struct, for example:
{ 'union': 'ExpirePasswordOptions',
'base': { 'protocol': 'DisplayProtocol',
'time': 'str' },
'discriminator': 'protocol',
'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
{ 'enum': 'DisplayProtocol',
'data': [ 'vnc', 'spice' ] }
generates:
type ExpirePasswordOptions struct {
Time string `json:"time"`
// Variants fields
Vnc *ExpirePasswordOptionsVnc `json:"-"`
// Unbranched enum fields
Spice bool `json:"-"`
}
if you want to navigate over it:
https://gitlab.com/victortoso/qapi-go/-/blob/qapi-golang-v2-by-tags/pkg/qapi/unions.go?ref_type=heads#L4516
> > +
> > + func (s ImageInfoSpecificQCow2Encryption) MarshalJSON() ([]byte, error) {
> > + // ...
> > + // Logic for branched Enum
> > + if s.Luks != nil && err == nil {
> > + if len(bytes) != 0 {
> > + err = errors.New(`multiple variant fields set`)
> > + } else if err = unwrapToMap(m, s.Luks); err == nil {
> > + m["format"] = BlockdevQcow2EncryptionFormatLuks
> > + bytes, err = json.Marshal(m)
> > + }
> > + }
> > +
> > + // Logic for unbranched Enum
> > + if s.Aes && err == nil {
> > + if len(bytes) != 0 {
> > + err = errors.New(`multiple variant fields set`)
> > + } else {
> > + m["format"] = BlockdevQcow2EncryptionFormatAes
> > + bytes, err = json.Marshal(m)
> > + }
> > + }
> > +
> > + // ...
> > + // Handle errors
> > + }
> > +
> > +
> > + func (s *ImageInfoSpecificQCow2Encryption) UnmarshalJSON(data []byte) error {
> > + // ...
> > +
> > + switch tmp.Format {
> > + case BlockdevQcow2EncryptionFormatLuks:
> > + s.Luks = new(QCryptoBlockInfoLUKS)
> > + if err := json.Unmarshal(data, s.Luks); err != nil {
> > + s.Luks = nil
> > + return err
> > + }
> > + case BlockdevQcow2EncryptionFormatAes:
> > + s.Aes = true
> > +
> > + default:
> > + return fmt.Errorf("error: unmarshal: ImageInfoSpecificQCow2Encryption: received unrecognized value: '%s'",
> > + tmp.Format)
> > + }
> > + return nil
> > + }
> > +
> > +
> > +Alternate
> > +---------
> > +
> > +Like Unions, alternates can have a few branches. Unlike Unions, they
>
> Scratch "a few".
Fixed
> > +don't have a discriminator field and each branch should be a different
> > +class of Type entirely (e.g: You can't have two branches of type int in
> > +one Alternate).
> > +
> > +While the marshalling is similar to Unions, the unmarshalling uses a
> > +try-and-error approach, trying to fit the data payload in one of the
> > +Alternate fields.
> > +
> > +The biggest caveat is handling Alternates that can take JSON Null as
> > +value. The issue lies on ``encoding/json`` library limitation where
> > +unmarshalling JSON Null data to a Go struct which has the 'omitempty'
> > +field that, it bypass the Marshal interface. The same happens when
> > +marshalling, if the field tag 'omitempty' is used, a nil pointer would
> > +never be translated to null JSON value.
> > +
> > +The problem being, we use pointer to type plus ``omitempty`` field to
> > +express a QAPI optional member.
> > +
> > +In order to handle JSON Null, the generator needs to do the following:
> > + - Read the QAPI schema prior to generate any code and cache
> > + all alternate types that can take JSON Null
> > + - For all Go structs that should be considered optional and they type
> > + are one of those alternates, do not set ``omitempty`` and implement
> > + Marshal interface for this Go struct, to properly handle JSON Null
> > + - In the Alternate, uses a boolean 'IsNull' to express a JSON Null
> > + and implement the AbsentAlternate interface, to help sturcts know
>
> Typo: to help structs
Thanks
> > + if a given Alternate type should be considered Absent (not set) or
> > + any other possible Value, including JSON Null.
> > +
> > +::
> > +
> > + { 'alternate': 'BlockdevRefOrNull',
> > + 'data': { 'definition': 'BlockdevOptions',
> > + 'reference': 'str',
> > + 'null': 'null' } }
> > +
> > +.. code-block:: go
> > +
> > + type BlockdevRefOrNull struct {
> > + Definition *BlockdevOptions
> > + Reference *string
> > + IsNull bool
> > + }
> > +
> > + func (s *BlockdevRefOrNull) ToAnyOrAbsent() (any, bool) {
> > + if s != nil {
> > + if s.IsNull {
> > + return nil, false
> > + } else if s.Definition != nil {
> > + return *s.Definition, false
> > + } else if s.Reference != nil {
> > + return *s.Reference, false
> > + }
> > + }
> > +
> > + return nil, true
> > + }
> > +
> > + func (s BlockdevRefOrNull) MarshalJSON() ([]byte, error) {
> > + if s.IsNull {
> > + return []byte("null"), nil
> > + } else if s.Definition != nil {
> > + return json.Marshal(s.Definition)
> > + } else if s.Reference != nil {
> > + return json.Marshal(s.Reference)
> > + }
> > + return []byte("{}"), nil
> > + }
> > +
> > + func (s *BlockdevRefOrNull) UnmarshalJSON(data []byte) error {
> > + // Check for json-null first
> > + if string(data) == "null" {
> > + s.IsNull = true
> > + return nil
> > + }
> > + // Check for BlockdevOptions
> > + {
> > + s.Definition = new(BlockdevOptions)
> > + if err := StrictDecode(s.Definition, data); err == nil {
> > + return nil
> > + }
> > + s.Definition = nil
> > + }
> > + // Check for string
> > + {
> > + s.Reference = new(string)
> > + if err := StrictDecode(s.Reference, data); err == nil {
> > + return nil
> > + }
> > + s.Reference = nil
> > + }
> > +
> > + return fmt.Errorf("Can't convert to BlockdevRefOrNull: %s", string(data))
> > + }
> > +
> > +
> > +Event
> > +-----
> > +
> > +All events are mapped to its own struct with the additional
>
> Each event is mapped to its own
Fixed
> > +MessageTimestamp field, for the over-the-wire 'timestamp' value.
> > +
> > +Marshaling and Unmarshaling happens over the Event interface, so users
> > +should use the MarshalEvent() and UnmarshalEvent() methods.
> > +
> > +::
> > +
> > + { 'event': 'SHUTDOWN',
> > + 'data': { 'guest': 'bool',
> > + 'reason': 'ShutdownCause' } }
> > +
> > +.. code-block:: go
> > +
> > + type Event interface {
> > + GetName() string
> > + GetTimestamp() Timestamp
> > + }
> > +
> > + type ShutdownEvent struct {
> > + MessageTimestamp Timestamp `json:"-"`
> > + Guest bool `json:"guest"`
> > + Reason ShutdownCause `json:"reason"`
> > + }
> > +
> > + func (s *ShutdownEvent) GetName() string {
> > + return "SHUTDOWN"
> > + }
> > +
> > + func (s *ShutdownEvent) GetTimestamp() Timestamp {
> > + return s.MessageTimestamp
> > + }
> > +
> > +
> > +Command
> > +-------
> > +
> > +All commands are mapped to its own struct with the additional MessageId
>
> Each command is mapped to its own
Fixed
> > +field for the optional 'id'. If the command has a boxed data struct,
> > +the option struct will be embed in the command struct.
> > +
> > +As commands do require a return value, every command has its own return
> > +type. The Command interface has a GetReturnType() method that returns a
> > +CommandReturn interface, to help Go application handling the data.
> > +
> > +Marshaling and Unmarshaling happens over the Command interface, so
> > +users should use the MarshalCommand() and UnmarshalCommand() methods.
> > +
> > +::
> > +
> > + { 'command': 'set_password',
> > + 'boxed': true,
> > + 'data': 'SetPasswordOptions' }
>
> Since you show the Go type generated for QAPI type SetPasswordOptions,
> you should show the QAPI type here.
Added.
> > +
> > +.. code-block:: go
> > +
> > + type Command interface {
> > + GetId() string
> > + GetName() string
> > + GetReturnType() CommandReturn
> > + }
> > +
> > + // SetPasswordOptions is embed
> > + type SetPasswordCommand struct {
> > + SetPasswordOptions
> > + MessageId string `json:"-"`
> > + }
> > +
> > + // This is an union
> > + type SetPasswordOptions struct {
> > + Protocol DisplayProtocol `json:"protocol"`
> > + Password string `json:"password"`
> > + Connected *SetPasswordAction `json:"connected,omitempty"`
> > +
> > + // Variants fields
> > + Vnc *SetPasswordOptionsVnc `json:"-"`
> > + }
> > +
> > +Now an example of a command without boxed type.
> > +
> > +::
> > +
> > + { 'command': 'set_link',
> > + 'data': {'name': 'str', 'up': 'bool'} }
> > +
> > +.. code-block:: go
> > +
> > + type SetLinkCommand struct {
> > + MessageId string `json:"-"`
> > + Name string `json:"name"`
> > + Up bool `json:"up"`
> > + }
> > +
> > +Known issues
> > +============
> > +
> > +- Type names might not follow proper Go convention. Andrea suggested an
> > + annotation to the QAPI schema that could solve it.
> > + https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg00127.html
Many thanks for the review,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/11] docs: add notes on Golang code generator
2023-10-18 16:21 ` Victor Toso
@ 2023-10-19 6:56 ` Markus Armbruster
0 siblings, 0 replies; 43+ messages in thread
From: Markus Armbruster @ 2023-10-19 6:56 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, John Snow, Daniel P . Berrangé, Andrea Bolognani
Victor Toso <victortoso@redhat.com> writes:
> Hi,
>
> On Wed, Oct 18, 2023 at 01:47:56PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > The goal of this patch is converge discussions into a documentation,
>> > to make it easy and explicit design decisions, known issues and what
>> > else might help a person interested in how the Go module is generated.
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> > docs/devel/index-build.rst | 1 +
>> > docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
>> > 2 files changed, 377 insertions(+)
>> > create mode 100644 docs/devel/qapi-golang-code-gen.rst
>> >
>> > diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
>> > index 57e8d39d98..8f7c6f5dc7 100644
>> > --- a/docs/devel/index-build.rst
>> > +++ b/docs/devel/index-build.rst
>> > @@ -15,5 +15,6 @@ the basics if you are adding new files and targets to the build.
>> > qtest
>> > ci
>> > qapi-code-gen
>> > + qapi-golang-code-gen
>> > fuzzing
>> > control-flow-integrity
>>
>> Let's not worry whether and how this should be integrated with
>> qapi-code-gen.rst for now.
>>
>> I'm a Go ignoramus. I hope my comments are at least somewhat
>> helpful all the same.
>
> They always are.
Thanks!
>> > diff --git a/docs/devel/qapi-golang-code-gen.rst b/docs/devel/qapi-golang-code-gen.rst
>> > new file mode 100644
>> > index 0000000000..b62daf3bad
>> > --- /dev/null
>> > +++ b/docs/devel/qapi-golang-code-gen.rst
>> > @@ -0,0 +1,376 @@
>> > +==========================
>> > +QAPI Golang code generator
>> > +==========================
>> > +
>> > +..
>> > + Copyright (C) 2023 Red Hat, Inc.
>> > +
>> > + This work is licensed under the terms of the GNU GPL, version 2 or
>> > + later. See the COPYING file in the top-level directory.
>> > +
>> > +
>> > +Introduction
>> > +============
>> > +
>> > +This document provides information of how the generated Go code maps
>> > +with the QAPI specification, clarifying design decisions when needed.
>> > +
>> > +
>> > +Scope of the generated Go code
>> > +==============================
>>
>> What do you mean by "scope"?
>
> To build an application to talk with QEMU over QMP, this
> generated code is not enough. What I mean is that, this is just
> the first layer. We still need a library on top to do the work of
> connecting, sending/receiving messages, etc.
>
> Any recommendations on how to word this better?
Yes: try working your explanation of layers into the text.
>> > +
>> > +The scope is limited to data structures that can interpret and be used
>> > +to generate valid QMP messages. These data structures are generated
>> > +from a QAPI schema and should be able to handle QMP messages from the
>> > +same schema.
>> > +
>> > +The generated Go code is a Go module with data structs that uses Go
>> > +standard library ``encoding/json``, implementing its field tags and
>> > +Marshal interface whenever needed.
>> > +
>> > +
>> > +QAPI types to Go structs
>> > +========================
>> > +
>> > +Enum
>> > +----
>> > +
>> > +Enums are mapped as strings in Go, using a specified string type per
>> > +Enum to help with type safety in the Go application.
>> > +
>> > +::
>> > +
>> > + { 'enum': 'HostMemPolicy',
>> > + 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
>> > +
>> > +.. code-block:: go
>> > +
>> > + type HostMemPolicy string
>> > +
>> > + const (
>> > + HostMemPolicyDefault HostMemPolicy = "default"
>> > + HostMemPolicyPreferred HostMemPolicy = "preferred"
>> > + HostMemPolicyBind HostMemPolicy = "bind"
>> > + HostMemPolicyInterleave HostMemPolicy = "interleave"
>> > + )
>> > +
>> > +
>> > +Struct
>> > +------
>> > +
>> > +The mapping between a QAPI struct in Go struct is very straightforward.
>> > + - Each member of the QAPI struct has its own field in a Go struct.
>> > + - Optional members are pointers type with 'omitempty' field tag set
>> > +
>> > +One important design decision was to _not_ embed base struct, copying
>> > +the base members to the original struct. This reduces the complexity
>> > +for the Go application.
>> > +
>> > +::
>> > +
>> > + { 'struct': 'BlockExportOptionsNbdBase',
>> > + 'data': { '*name': 'str', '*description': 'str' } }
>> > +
>> > + { 'struct': 'BlockExportOptionsNbd',
>> > + 'base': 'BlockExportOptionsNbdBase',
>> > + 'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
>> > + '*allocation-depth': 'bool' } }
>> > +
>> > +.. code-block:: go
>> > +
>> > + type BlockExportOptionsNbd struct {
>> > + Name *string `json:"name,omitempty"`
>> > + Description *string `json:"description,omitempty"`
>> > +
>> > + Bitmaps []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
>> > + AllocationDepth *bool `json:"allocation-depth,omitempty"`
>> > + }
>>
>> Is there a need to convert from type to base type?
>
> Do you mean why we don't embed a base struct and do a copy of its
> fields instead? If yes, the main reason is aesthetic. See this
> issue: https://github.com/golang/go/issues/29438
>
> So, we could have a situation where we have a embed type, inside
> a embed type, inside of embed type making it horrible to write
> the code for using it.
Yes, avoiding such horrors is why we copy the base type's members
generated C.
In object oriented languages with inheritance, you can accomplish the
same by inheriting the base type. These languages commonly let you
convert to the base type then.
When copying the base type's members, converting to the base type may
require extra code. It doesn't in C:
>> In C, we get away with (BlockExportOptionsNbdBase *)obj. We hide it in
>> an inline function, though.
I don't actually know whether it does in Go. Only matters if there's an
actual need for such a conversion, hence my question.
>> > +
>> > +
>> > +Union
>> > +-----
>> > +
>> > +Unions in QAPI are binded to a Enum type which provides all possible
>>
>> bound to an Enum
>
> Fixed.
>
>> > +branches of the union. The most important caveat here is that the Union
>> > +does not need to have a complex type implemented for all possible
>> > +branches of the Enum. Receiving a enum value of a unimplemented branch
>>
>> I'd call that branch empty, not unimplemented.
>
> Ok, changed them all to empty branch.
>
>> > +is valid.
>> > +
>> > +For this reason, the generated Go struct will define a field for each
>> > +Enum value. The Go type defined for unbranched Enum values is bool
>>
>> Important design decision: since Go sucks at sum types, you elect to add
>> all the variant members to the struct. Only one of them may be used at
>> any time. Please spell that out more clearly.
>
> What about:
>
> The generated Go struct will then define a field for each
> Enum value. The type for Enum values of empty branch is bool.
> Only one field can be set at time.
Better.
We'll polish the prose later.
>> > +
>> > +Go struct and also implement the Marshal interface.
>>
>> Blank line in the middle of a sentence? Or two sentences? Can't make
>> sense of them.
>
> Leftover when rewriting it. Removed it.
>
>> What do you mean by "unbranched" Enum value? Enum value with an
>> implicit (empty) branch?
>
> Yes, changing it to empty branch or empty branched.
>
>> > +
>> > +As each Union Go struct type has both the discriminator field and
>> > +optional fields, it is important to note that when converting Go struct
>> > +to JSON, we only consider the discriminator field if no optional field
>> > +member was set. In practice, the user should use the optional fields if
>> > +the QAPI Union type has defined them, otherwise the user can set the
>> > +discriminator field for the unbranched enum value.
>>
>> I don't think I get this paragraph.
>
> Sorry, leftover again. I've removed it entirely.
>
> This bit was when we had a Discriminator field, we don't have it
> anymore.
>
>> > +
>> > +::
>> > +
>> > + { 'union': 'ImageInfoSpecificQCow2Encryption',
>> > + 'base': 'ImageInfoSpecificQCow2EncryptionBase',
>> > + 'discriminator': 'format',
>> > + 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
>>
>> The example is hard to understand without additional context, namely:
>>
>> { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>> 'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>>
>> { 'enum': 'BlockdevQcow2EncryptionFormat',
>> 'data': [ 'aes', 'luks' ] }
>
> Added.
>
>> > +
>> > +.. code-block:: go
>> > +
>> > + type ImageInfoSpecificQCow2Encryption struct {
>> > + // Variants fields
>> > + Luks *QCryptoBlockInfoLUKS `json:"-"`
>> > + // Unbranched enum fields
>> > + Aes bool `json:"-"`
>> > + }
>>
>> The members of the base type are the common members, and the members of
>> the branch's type are the variant members.
>>
>> Your example shows the variant member: 'luks'.
>>
>> The common member @format isn't there. I guess you're eliding it
>> because you can derive its value from other members.
>
> Correct. We can define @format value based on user's setting Aes
> or Luks.
>
>> If there were other common members, where would they go?
>
> They should all be at the top of the struct, for example:
>
> { 'union': 'ExpirePasswordOptions',
> 'base': { 'protocol': 'DisplayProtocol',
> 'time': 'str' },
> 'discriminator': 'protocol',
> 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>
> { 'enum': 'DisplayProtocol',
> 'data': [ 'vnc', 'spice' ] }
>
> generates:
>
> type ExpirePasswordOptions struct {
> Time string `json:"time"`
> // Variants fields
> Vnc *ExpirePasswordOptionsVnc `json:"-"`
> // Unbranched enum fields
> Spice bool `json:"-"`
> }
>
> if you want to navigate over it:
>
> https://gitlab.com/victortoso/qapi-go/-/blob/qapi-golang-v2-by-tags/pkg/qapi/unions.go?ref_type=heads#L4516
Recommend to use an example that lets us show Go for such common members.
[...]
>
> Many thanks for the review,
> Victor
You're welcome!
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (10 preceding siblings ...)
2023-10-16 15:27 ` [PATCH v2 11/11] docs: add notes on Golang code generator Victor Toso
@ 2023-10-27 17:33 ` Victor Toso
2023-10-31 16:42 ` Andrea Bolognani
2023-11-09 18:35 ` Andrea Bolognani
12 siblings, 1 reply; 43+ messages in thread
From: Victor Toso @ 2023-10-27 17:33 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Daniel P . Berrangé,
Andrea Bolognani
[-- Attachment #1: Type: text/plain, Size: 3463 bytes --]
Hi,
Daniel & Andrea, it would be great to have your take on the Go
side of this series. If we can agree with an acceptable
'unstable' version of Go modules, we can start building on top of
this:
- libraries/tools in Go to interact with QEMU
- qapi specs to fix limitations (e.g: Data type names)
- scripts/qapi library wrt to generating interfaces in other
languages other than C
I would love to have this prior to 8.2 feature freeze if the
idea and current code meet your expectations.
Cheers,
Victor
On Mon, Oct 16, 2023 at 05:26:53PM +0200, Victor Toso wrote:
> This patch series intent is to introduce a generator that produces a Go
> module for Go applications to interact over QMP with QEMU.
>
> This is the second iteration:
> v1: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06734.html
>
> I've pushed this series in my gitlab fork:
> https://gitlab.com/victortoso/qemu/-/tree/qapi-golang-v2
>
> I've also generated the qapi-go module over QEMU tags: v7.0.0, v7.1.0,
> v7.2.6, v8.0.0 and v8.1.0, see the commits history here:
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-tags
>
> I've also generated the qapi-go module over each commit of this series,
> see the commits history here (using previous refered qapi-golang-v2)
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-patch
>
> Cheers,
> Victor
>
> * Changes:
> - All patches were rebased using black python formatting tool, awesome.
> (John) https://black.readthedocs.io/en/stable/
> - All patches were tested with flake8 and pylint. Minor complains
> remains. (John)
> - All generated types are sorted in alphabetical order (Daniel)
> - Using utf8 instead of ascii encoding of output files
> - Improved commit logs
> - renamed QapiError -> QAPIError (Daniel)
> - QAPIError's Error() returns only the description (Daniel)
> - Used more type hints (Where I could) (John)
> - Removed typehint from self in the Class implementation (John)
> - The Go code that is generated is now well formated. gofmt -w and
> goimport -w don't change a thing.
> - Added a fix from John
> https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01305.html
> - Added a tangent fix suggestion to main.py "scripts: qapi: black
> format main.py"
>
> John Snow (1):
> qapi: re-establish linting baseline
>
> Victor Toso (10):
> scripts: qapi: black format main.py
> qapi: golang: Generate qapi's enum types in Go
> qapi: golang: Generate qapi's alternate types in Go
> qapi: golang: Generate qapi's struct types in Go
> qapi: golang: structs: Address 'null' members
> qapi: golang: Generate qapi's union types in Go
> qapi: golang: Generate qapi's event types in Go
> qapi: golang: Generate qapi's command types in Go
> qapi: golang: Add CommandResult type to Go
> docs: add notes on Golang code generator
>
> docs/devel/index-build.rst | 1 +
> docs/devel/qapi-golang-code-gen.rst | 376 ++++++++
> scripts/qapi/gen.py | 2 +-
> scripts/qapi/golang.py | 1349 +++++++++++++++++++++++++++
> scripts/qapi/main.py | 79 +-
> scripts/qapi/parser.py | 5 +-
> 6 files changed, 1781 insertions(+), 31 deletions(-)
> create mode 100644 docs/devel/qapi-golang-code-gen.rst
> create mode 100644 scripts/qapi/golang.py
>
> --
> 2.41.0
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-10-27 17:33 ` [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
@ 2023-10-31 16:42 ` Andrea Bolognani
2023-11-03 18:34 ` Andrea Bolognani
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-10-31 16:42 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Fri, Oct 27, 2023 at 07:33:30PM +0200, Victor Toso wrote:
> Hi,
>
> Daniel & Andrea, it would be great to have your take on the Go
> side of this series. If we can agree with an acceptable
> 'unstable' version of Go modules, we can start building on top of
> this:
> - libraries/tools in Go to interact with QEMU
> - qapi specs to fix limitations (e.g: Data type names)
> - scripts/qapi library wrt to generating interfaces in other
> languages other than C
>
> I would love to have this prior to 8.2 feature freeze if the
> idea and current code meet your expectations.
Apologies for not providing any feedback so far. I'll do my best to
get around to it by the end of the week.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-10-31 16:42 ` Andrea Bolognani
@ 2023-11-03 18:34 ` Andrea Bolognani
2023-11-06 12:00 ` Victor Toso
0 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-03 18:34 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Tue, Oct 31, 2023 at 09:42:10AM -0700, Andrea Bolognani wrote:
> On Fri, Oct 27, 2023 at 07:33:30PM +0200, Victor Toso wrote:
> > Hi,
> >
> > Daniel & Andrea, it would be great to have your take on the Go
> > side of this series. If we can agree with an acceptable
> > 'unstable' version of Go modules, we can start building on top of
> > this:
> > - libraries/tools in Go to interact with QEMU
> > - qapi specs to fix limitations (e.g: Data type names)
> > - scripts/qapi library wrt to generating interfaces in other
> > languages other than C
> >
> > I would love to have this prior to 8.2 feature freeze if the
> > idea and current code meet your expectations.
>
> Apologies for not providing any feedback so far. I'll do my best to
> get around to it by the end of the week.
Layering apologies on top of apologies: I started looking into this,
but I have since realized that I need some more time to page all the
months-old context back in and digest the whole thing. I'll continue
next week.
As an appetizer, one thing that I've noticed: you seem to ignore it
when gen:false is included in a command definition, so we get
type DeviceAddCommand struct {
MessageId string `json:"-"`
Driver string `json:"driver"`
Bus *string `json:"bus,omitempty"`
Id *string `json:"id,omitempty"`
}
which I don't think will work as it can't handle even the example
used to document the command
{ "execute": "device_add",
"arguments": { "driver": "e1000", "id": "net1",
"bus": "pci.0",
"mac": "52:54:00:12:34:56" } }
This command will probably require an ad-hoc implementation.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-11-03 18:34 ` Andrea Bolognani
@ 2023-11-06 12:00 ` Victor Toso
0 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-11-06 12:00 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]
Hi,
On Fri, Nov 03, 2023 at 11:34:18AM -0700, Andrea Bolognani wrote:
> On Tue, Oct 31, 2023 at 09:42:10AM -0700, Andrea Bolognani wrote:
> > On Fri, Oct 27, 2023 at 07:33:30PM +0200, Victor Toso wrote:
> > > Hi,
> > >
> > > Daniel & Andrea, it would be great to have your take on the Go
> > > side of this series. If we can agree with an acceptable
> > > 'unstable' version of Go modules, we can start building on top of
> > > this:
> > > - libraries/tools in Go to interact with QEMU
> > > - qapi specs to fix limitations (e.g: Data type names)
> > > - scripts/qapi library wrt to generating interfaces in other
> > > languages other than C
> > >
> > > I would love to have this prior to 8.2 feature freeze if the
> > > idea and current code meet your expectations.
> >
> > Apologies for not providing any feedback so far. I'll do my best to
> > get around to it by the end of the week.
>
> Layering apologies on top of apologies: I started looking into this,
> but I have since realized that I need some more time to page all the
> months-old context back in and digest the whole thing. I'll continue
> next week.
>
> As an appetizer, one thing that I've noticed: you seem to ignore it
> when gen:false is included in a command definition, so we get
>
> type DeviceAddCommand struct {
> MessageId string `json:"-"`
> Driver string `json:"driver"`
> Bus *string `json:"bus,omitempty"`
> Id *string `json:"id,omitempty"`
> }
>
> which I don't think will work as it can't handle even the example
> used to document the command
>
> { "execute": "device_add",
> "arguments": { "driver": "e1000", "id": "net1",
> "bus": "pci.0",
> "mac": "52:54:00:12:34:56" } }
>
> This command will probably require an ad-hoc implementation.
Indeed, thanks for catching this.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-10-16 15:26 [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
` (11 preceding siblings ...)
2023-10-27 17:33 ` [PATCH v2 00/11] qapi-go: add generator for Golang interface Victor Toso
@ 2023-11-09 18:35 ` Andrea Bolognani
2023-11-09 19:03 ` Victor Toso
12 siblings, 1 reply; 43+ messages in thread
From: Andrea Bolognani @ 2023-11-09 18:35 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Mon, Oct 16, 2023 at 05:26:53PM +0200, Victor Toso wrote:
> This patch series intent is to introduce a generator that produces a Go
> module for Go applications to interact over QMP with QEMU.
>
> This is the second iteration:
> v1: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06734.html
>
> I've pushed this series in my gitlab fork:
> https://gitlab.com/victortoso/qemu/-/tree/qapi-golang-v2
>
> I've also generated the qapi-go module over QEMU tags: v7.0.0, v7.1.0,
> v7.2.6, v8.0.0 and v8.1.0, see the commits history here:
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-tags
>
> I've also generated the qapi-go module over each commit of this series,
> see the commits history here (using previous refered qapi-golang-v2)
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-patch
I've provided feedback on the various facets of the API in response
to the corresponding patch. Note that I've only addressed concerns
about the consumer-facing API: I have some notes about the
implementation as well, but that's something that we should be able
to easily change transparently so I didn't give it priority.
Overall, I think that the current API is quite close to being a solid
PoC that can be used as a starting point for further development.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
2023-11-09 18:35 ` Andrea Bolognani
@ 2023-11-09 19:03 ` Victor Toso
0 siblings, 0 replies; 43+ messages in thread
From: Victor Toso @ 2023-11-09 19:03 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
Hi,
On Thu, Nov 09, 2023 at 10:35:07AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:26:53PM +0200, Victor Toso wrote:
> > This patch series intent is to introduce a generator that produces a Go
> > module for Go applications to interact over QMP with QEMU.
> >
> > This is the second iteration:
> > v1: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06734.html
> >
> > I've pushed this series in my gitlab fork:
> > https://gitlab.com/victortoso/qemu/-/tree/qapi-golang-v2
> >
> > I've also generated the qapi-go module over QEMU tags: v7.0.0, v7.1.0,
> > v7.2.6, v8.0.0 and v8.1.0, see the commits history here:
> > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-tags
> >
> > I've also generated the qapi-go module over each commit of this series,
> > see the commits history here (using previous refered qapi-golang-v2)
> > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-patch
>
> I've provided feedback on the various facets of the API in response
> to the corresponding patch. Note that I've only addressed concerns
> about the consumer-facing API: I have some notes about the
> implementation as well, but that's something that we should be able
> to easily change transparently so I didn't give it priority.
Sure thing.
> Overall, I think that the current API is quite close to being a
> solid PoC that can be used as a starting point for further
> development.
Happy o hear it. Many thanks for the review, I really appreciate
it.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread