* [RFC PATCH v2 0/8] qapi: add generator for Golang interface
@ 2022-06-17 12:19 Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 1/8] qapi: golang: Generate qapi's enum types in Go Victor Toso
` (9 more replies)
0 siblings, 10 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
Hi,
This is the second iteration of RFC v1:
https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html
# What this is about?
To generate a simple Golang interface that could communicate with QEMU
over QMP. The Go code that is generated is meant to be used as the bare
bones to exchange QMP messages.
The goal is to have this as a Go module in QEMU gitlab namespace,
similar to what have been done to pyhon-qemu-qmp
https://gitlab.com/qemu-project/python-qemu-qmp
# Issues raised in RFC v1
The leading '*' for issues I addressed in this iteration
* 1) Documentation was removed to avoid License issues, by Daniel
Thread: https://lists.nongnu.org/archive/html/qemu-devel/2022-05/msg01889.html
It is important for the generated Go module to be compatible with
Licenses used by projects that would be using this. Copying the
documentation of the QAPI spec might conflict with GPLv2+.
I have not proposed another license in this iteration, but I'm
planning to go with MIT No Attribution, aka MIT-0 [0]. Does it make
sense to bind the generated code's license to MIT-0 already at
generator level?
[0] https://github.com/aws/mit-0/blob/master/MIT-0
2) Inconsistent generated Names, by Andrea + Markus
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg05026.html
Example 1:
| qapi | Go | Expected |
| @logappend | Logappend | LogAppend |
Example 2:
(acronyms) VncInfo and DisplayReloadOptionsVNC
This was not addressed in RFC v2 mainly because it seems to need
more metadata support from the QAPI spec to handle specific
scenarios. The solution seems either an extra metadata proposal by
Andrea [1] or reviving Kevin's work [2]
[1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg00127.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg04703.html
* 3) Better type safety, by Andrea + Daniel
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01906.html
Most of the 'Any' type (interface {}) has been removed. The only
place it still exists is for fields that uses QAPI's any type, like
with command qom-set or the struct type ObjectPropertyInfo.
* 4) QAPI enums mapped to String instead of Int type, by Daniel.
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01904.html
I'm still over the fence about using string here, mostly by the
same issue reported here:
https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/30#note_975517740
* 5) Events and Commands as interface, by Daniel
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01914.html
So, instead of having a Command/Event struct with a Any type for
the Arguments (which could be set with SetPasswordCommand struct
type for example); now we have a Command interface which all
previous structs that behaved as Arguments implement.
I've included Marshal{Command Event} and Unmarshal{Command Event}
helper functions that operate on top of each interface.
* 6) Removing Any from Unions, by Daniel
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01912.html
I basically followed the above suggestion to all other types that
used Any. Specifically to unions were the removal of the
'discriminator' field, as proposed also in the above link.
* 7) Flat structs by removing embed types. Discussion with Andrea
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
No one required it but I decided to give it a try. Major issue that
I see with this approach is to have generated a few 'Base' structs
that are now useless. Overall, less nested structs seems better to
me. Opnions?
Example:
| /* This is now useless, should be removed? */
| type InetSocketAddressBase struct {
| Host string `json:"host"`
| Port string `json:"port"`
| }
|
| type InetSocketAddress struct {
| // Base fields
| Host string `json:"host"`
| Port string `json:"port"`
|
|
| Numeric *bool `json:"numeric,omitempty"`
| To *uint16 `json:"to,omitempty"`
| Ipv4 *bool `json:"ipv4,omitempty"`
| Ipv6 *bool `json:"ipv6,omitempty"`
| KeepAlive *bool `json:"keep-alive,omitempty"`
| Mptcp *bool `json:"mptcp,omitempty"`
| }
8) Supporting multiple versions
Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg02147.html
I'm keen to working on the proposed solution above as it seems a
good compromise to make code that can be compatible with multiple
versions of qmp/qemu.
But the basis needs to be defined first, so this is for the future.
* 9) Handling { "error": { ... } }
This was missing in the RFC v1. I've added a QapiError type that is
included in all CommandReturn types, following Andrea's suggestion:
https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg02199.html
# victortoso/qapi-go and victortoso/qemu
I'm currently hosting the generated code from this RFCv2 at:
https://gitlab.com/victortoso/qapi-go/-/tree/main/pkg/qapi
This series can also be found at:
https://gitlab.com/victortoso/qemu/-/commits/qapi-golang
Thanks for taking a look,
Victor
Victor Toso (8):
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: 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
qapi: golang: document skip function visit_array_types
scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++
scripts/qapi/main.py | 2 +
2 files changed, 767 insertions(+)
create mode 100644 scripts/qapi/golang.py
--
2.36.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC PATCH v2 1/8] qapi: golang: Generate qapi's enum types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate " Victor Toso
` (8 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
This patch handles QAPI enum types and generates its equivalent in Go.
Basically, 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 [0], which is everything.
[0] https://go.dev/ref/spec#Exported_identifiers
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 | 134 +++++++++++++++++++++++++++++++++++++++++
scripts/qapi/main.py | 2 +
2 files changed, 136 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..f2776520a1
--- /dev/null
+++ b/scripts/qapi/golang.py
@@ -0,0 +1,134 @@
+"""
+Golang QAPI generator
+"""
+# Copyright (c) 2022 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,
+ QAPISchemaType,
+ QAPISchemaVisitor,
+ QAPISchemaEnumMember,
+ QAPISchemaFeature,
+ QAPISchemaIfCond,
+ QAPISchemaObjectType,
+ QAPISchemaObjectTypeMember,
+ QAPISchemaVariants,
+)
+from .source import QAPISourceInfo
+
+
+class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
+
+ def __init__(self, prefix: str):
+ super().__init__()
+ self.target = {name: "" for name in ["enum"]}
+ self.schema = None
+ self.golang_package_name = "qapi"
+
+ def visit_begin(self, schema):
+ 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):
+ self.schema = None
+
+ def visit_object_type(self: QAPISchemaGenGolangVisitor,
+ 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: QAPISchemaGenGolangVisitor,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ variants: QAPISchemaVariants
+ ) -> None:
+ pass
+
+ def visit_enum_type(self: QAPISchemaGenGolangVisitor,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ members: List[QAPISchemaEnumMember],
+ prefix: Optional[str]
+ ) -> None:
+
+ value = qapi_to_field_name_enum(members[0].name)
+ fields = ""
+ for member in members:
+ value = qapi_to_field_name_enum(member.name)
+ fields += f'''\t{name}{value} {name} = "{member.name}"\n'''
+
+ self.target["enum"] += f'''
+type {name} string
+const (
+{fields[:-1]}
+)
+'''
+
+ def visit_array_type(self, name, info, ifcond, element_type):
+ 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, info, ifcond, features, arg_type, boxed):
+ 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") as outfile:
+ outfile.write(content)
+
+
+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("-", "")
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index fc216a53d3..661fb1e091 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
@@ -54,6 +55,7 @@ def generate(schema_file: str,
gen_events(schema, output_dir, prefix)
gen_introspect(schema, output_dir, prefix, unmask)
+ gen_golang(schema, output_dir, prefix)
def main() -> int:
"""
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 1/8] qapi: golang: Generate qapi's enum types in Go Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-09-02 14:49 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct " Victor Toso
` (7 subsequent siblings)
9 siblings, 2 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
This patch handles QAPI alternate types and generates data structures
in Go that handles it.
At this moment, there are 5 alternates in qemu/qapi, they are:
* BlockDirtyBitmapMergeSource
* Qcow2OverlapChecks
* BlockdevRef
* BlockdevRefOrNull
* StrOrNull
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, all 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"
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 119 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 117 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index f2776520a1..37d7c062c9 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -29,11 +29,32 @@
from .source import QAPISourceInfo
+TEMPLATE_HELPER = '''
+// Alias for go version lower than 1.18
+type Any = interface{}
+
+// Creates a decoder that errors on unknown Fields
+// Returns true if successfully decoded @from string @into type
+// Returns false without error is failed with "unknown field"
+// Returns false with error is a different error was found
+func StrictDecode(into interface{}, from []byte) error {
+ dec := json.NewDecoder(strings.NewReader(string(from)))
+ dec.DisallowUnknownFields()
+
+ if err := dec.Decode(into); err != nil {
+ return err
+ }
+ return nil
+}
+'''
+
+
class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
def __init__(self, prefix: str):
super().__init__()
- self.target = {name: "" for name in ["enum"]}
+ self.target = {name: "" for name in ["alternate", "enum", "helper"]}
+ self.objects_seen = {}
self.schema = None
self.golang_package_name = "qapi"
@@ -44,6 +65,8 @@ def visit_begin(self, schema):
for target in self.target:
self.target[target] = f"package {self.golang_package_name}\n"
+ self.target["helper"] += TEMPLATE_HELPER
+
def visit_end(self):
self.schema = None
@@ -65,7 +88,69 @@ def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants
) -> None:
- pass
+ assert name not in self.objects_seen
+ self.objects_seen[name] = True
+
+ marshal_return_default = f'nil, errors.New("{name} has empty fields")'
+ marshal_check_fields = ""
+ unmarshal_check_fields = ""
+ variant_fields = ""
+
+ # We need to check if the Alternate type supports NULL as that
+ # means that JSON to Go would allow all fields to be empty.
+ # Alternate that don't support NULL, would fail to convert
+ # to JSON if all fields were empty.
+ return_on_null = f"errors.New(`null not supported for {name}`)"
+
+ # Assembly the fields and all the checks for Marshal and
+ # Unmarshal methods
+ for var in variants.variants:
+ # Nothing to generate on null types. We update some
+ # variables to handle json-null on marshalling methods.
+ if var.type.name == "null":
+ marshal_return_default = '[]byte("null"), nil'
+ return_on_null = "nil"
+ continue
+
+ var_name = qapi_to_field_name(var.name)
+ var_type = qapi_schema_type_to_go_type(var.type.name)
+ variant_fields += f"\t{var_name} *{var_type}\n"
+
+ if len(marshal_check_fields) > 0:
+ marshal_check_fields += "} else "
+
+ marshal_check_fields += f'''if s.{var_name} != nil {{
+ return json.Marshal(s.{var_name})
+ '''
+
+ unmarshal_check_fields += f'''// Check for {var_type}
+ {{
+ s.{var_name} = new({var_type})
+ if err := StrictDecode(s.{var_name}, data); err == nil {{
+ return nil
+ }}
+ s.{var_name} = nil
+ }}
+'''
+
+ marshal_check_fields += "}"
+
+ self.target["alternate"] += generate_struct_type(name, variant_fields)
+ self.target["alternate"] += f'''
+func (s {name}) MarshalJSON() ([]byte, error) {{
+ {marshal_check_fields}
+ return {marshal_return_default}
+}}
+
+func (s *{name}) UnmarshalJSON(data []byte) error {{
+ // Check for json-null first
+ if string(data) == "null" {{
+ return {return_on_null}
+ }}
+ {unmarshal_check_fields}
+ return errors.New(fmt.Sprintf("Can't convert to {name}: %s", string(data)))
+}}
+'''
def visit_enum_type(self: QAPISchemaGenGolangVisitor,
name: str,
@@ -130,5 +215,35 @@ def gen_golang(schema: QAPISchema,
vis.write(output_dir)
+# Helper function for boxed or self contained structures.
+def generate_struct_type(type_name, args="") -> str:
+ args = args if len(args) == 0 else f"\n{args}\n"
+ return f'''
+type {type_name} struct {{{args}}}
+'''
+
+
+def qapi_schema_type_to_go_type(type: 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 type.endswith("List"):
+ prefix = "[]"
+ type = type[:-4]
+
+ type = schema_types_to_go.get(type, type)
+ return prefix + type
+
+
def qapi_to_field_name_enum(name: str) -> str:
return name.title().replace("-", "")
+
+
+def qapi_to_field_name(name: str) -> str:
+ return name.title().replace("_", "").replace("-", "")
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 1/8] qapi: golang: Generate qapi's enum types in Go Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate " Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-06-17 14:41 ` Daniel P. Berrangé
2022-06-17 12:19 ` [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union " Victor Toso
` (6 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
This patch handles QAPI struct types and generates the equivalent
types in Go.
At the time of this writing, it generates 388 structures.
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.
The downside are some generated Types that are likely useless now,
like InetSocketAddressBase from InetSocketAddress.
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.
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 ordecoding, 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 | 117 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 115 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 37d7c062c9..1ab0c0bb46 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -53,7 +53,7 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
def __init__(self, prefix: str):
super().__init__()
- self.target = {name: "" for name in ["alternate", "enum", "helper"]}
+ self.target = {name: "" for name in ["alternate", "enum", "helper", "struct"]}
self.objects_seen = {}
self.schema = None
self.golang_package_name = "qapi"
@@ -79,7 +79,37 @@ def visit_object_type(self: QAPISchemaGenGolangVisitor,
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]
) -> None:
- pass
+ # Do not handle anything besides structs
+ if (name == self.schema.the_empty_object_type.name or
+ not isinstance(name, str) or
+ info.defn_meta not in ["struct"]):
+ return
+
+ # Safety checks.
+ assert name not in self.objects_seen
+ self.objects_seen[name] = True
+
+ # 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.target[info.defn_meta] += qapi_to_golang_struct(name,
+ info,
+ ifcond,
+ features,
+ base,
+ members,
+ variants)
def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
name: str,
@@ -223,6 +253,72 @@ def generate_struct_type(type_name, args="") -> str:
'''
+# Helper function that is used for most of QAPI types
+def qapi_to_golang_struct(name: str,
+ info: Optional[QAPISourceInfo],
+ ifcond: QAPISchemaIfCond,
+ features: List[QAPISchemaFeature],
+ base: Optional[QAPISchemaObjectType],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants]) -> str:
+
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+
+ fields = ""
+
+ if base:
+ base_fields = ""
+ for lm in base.local_members:
+ # We do not include the Union's discriminator
+ # into the generated Go struct as the selection
+ # of what variant was set is enough on Go side.
+ if variants and variants.tag_member.name == lm.name:
+ continue
+
+ field = qapi_to_field_name(lm.name)
+ member_type = qapi_schema_type_to_go_type(lm.type.name)
+
+ isptr = "*" if lm.optional and member_type[0] not in "*[" else ""
+ optional = ",omitempty" if lm.optional else ""
+ fieldtag = f'`json:"{lm.name}{optional}"`'
+
+ base_fields += f"\t{field} {isptr}{member_type}{fieldtag}\n"
+
+ if len(base_fields) > 0:
+ fields += f"\t// Base fields\n\t{base_fields}\n"
+
+ if members:
+ for memb in members:
+ field = qapi_to_field_name(memb.name)
+ member_type = qapi_schema_type_to_go_type(memb.type.name)
+
+ isptr = "*" if memb.optional and member_type[0] not in "*[" else ""
+ optional = ",omitempty" if memb.optional else ""
+ fieldtag = f'`json:"{memb.name}{optional}"`'
+
+ fields += f"\t{field} {isptr}{member_type}{fieldtag}\n"
+
+ fields += "\n"
+
+ if variants:
+ fields += "\t// Variants fields\n"
+ for var in variants.variants:
+ if var.type.is_implicit():
+ continue
+
+ field = qapi_to_field_name(var.name)
+ member_type = qapi_schema_type_to_go_type(var.type.name)
+ # Variant's are handled in the Marshal/Unmarshal methods
+ fieldtag = '`json:"-"`'
+ fields += f"\t{field} *{member_type}{fieldtag}\n"
+ member_type = qapi_schema_type_to_go_type(var.type.name)
+ # Variant's are handled in the Marshal/Unmarshal methods
+ fieldtag = '`json:"-"`'
+ fields += f"\t{field} *{member_type}{fieldtag}\n"
+
+ return generate_struct_type(type_name, fields)
+
+
def qapi_schema_type_to_go_type(type: str) -> str:
schema_types_to_go = {
'str': 'string', 'null': 'nil', 'bool': 'bool', 'number':
@@ -247,3 +343,20 @@ def qapi_to_field_name_enum(name: str) -> str:
def qapi_to_field_name(name: str) -> str:
return name.title().replace("_", "").replace("-", "")
+
+
+def qapi_to_go_type_name(name: str, meta: str) -> str:
+ if name.startswith("q_obj_"):
+ 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 = [word for word in 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
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (2 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct " Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-06-17 12:19 ` [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event " Victor Toso
` (5 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
This patch handles QAPI union types and generates the equivalent data
structures and methods in Go to handle it.
At the moment of this writing, it generates 38 structures.
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. The @discriminator is not used in the
generated union Go structs as it suffices to check which one of the
@variants fields were set.
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 {
| // Base fields
| Password string `json:"password"`
| Connected *SetPasswordAction `json:"connected,omitempty"`
|
| // Variants fields
| Vnc *SetPasswordOptionsVnc `json:"-"`
| }
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 112 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 105 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 1ab0c0bb46..6c6a5cea97 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -53,7 +53,8 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
def __init__(self, prefix: str):
super().__init__()
- self.target = {name: "" for name in ["alternate", "enum", "helper", "struct"]}
+ self.target = {name: "" for name in ["alternate", "enum", "helper", "struct",
+ "union"]}
self.objects_seen = {}
self.schema = None
self.golang_package_name = "qapi"
@@ -79,10 +80,14 @@ def visit_object_type(self: QAPISchemaGenGolangVisitor,
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]
) -> None:
- # Do not handle anything besides structs
+ # 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"]):
+ info.defn_meta not in ["struct", "union"]):
+ return
+
+ # Base structs are embed
+ if qapi_name_is_base(name):
return
# Safety checks.
@@ -110,6 +115,10 @@ def visit_object_type(self: QAPISchemaGenGolangVisitor,
base,
members,
variants)
+ if info.defn_meta == "union":
+ self.target[info.defn_meta] += qapi_to_golang_methods_union(name,
+ info,
+ variants)
def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
name: str,
@@ -311,14 +320,99 @@ def qapi_to_golang_struct(name: str,
# Variant's are handled in the Marshal/Unmarshal methods
fieldtag = '`json:"-"`'
fields += f"\t{field} *{member_type}{fieldtag}\n"
- member_type = qapi_schema_type_to_go_type(var.type.name)
- # Variant's are handled in the Marshal/Unmarshal methods
- fieldtag = '`json:"-"`'
- fields += f"\t{field} *{member_type}{fieldtag}\n"
return generate_struct_type(type_name, fields)
+def qapi_to_golang_methods_union(name: str,
+ info: Optional[QAPISourceInfo],
+ variants: Optional[QAPISchemaVariants]
+ ) -> str:
+
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+
+ driverCases = ""
+ checkFields = ""
+ if variants:
+ for var in variants.variants:
+ if var.type.is_implicit():
+ continue
+
+ field = qapi_to_field_name(var.name)
+ member_type = qapi_schema_type_to_go_type(var.type.name)
+
+ if len(checkFields) > 0:
+ checkFields += "\t} else "
+ checkFields += f'''if s.{field} != nil {{
+ driver = "{var.name}"
+ payload, err = json.Marshal(s.{field})
+'''
+ # for Unmarshal method
+ driverCases += f'''
+ case "{var.name}":
+ s.{field} = new({member_type})
+ if err := json.Unmarshal(data, s.{field}); err != nil {{
+ s.{field} = nil
+ return err
+ }}'''
+
+ checkFields += "}"
+
+ return f'''
+func (s {type_name}) MarshalJSON() ([]byte, error) {{
+ type Alias {type_name}
+ alias := Alias(s)
+ base, err := json.Marshal(alias)
+ if err != nil {{
+ return nil, err
+ }}
+
+ driver := ""
+ payload := []byte{{}}
+ {checkFields}
+
+ if err != nil {{
+ return nil, err
+ }}
+
+ if len(base) == len("{{}}") {{
+ base = []byte("")
+ }} else {{
+ base = append([]byte{{','}}, base[1:len(base)-1]...)
+ }}
+
+ if len(payload) == 0 || len(payload) == len("{{}}") {{
+ payload = []byte("")
+ }} else {{
+ payload = append([]byte{{','}}, payload[1:len(payload)-1]...)
+ }}
+
+ result := fmt.Sprintf(`{{"{variants.tag_member.name}":"%s"%s%s}}`, driver, base, payload)
+ return []byte(result), nil
+}}
+
+func (s *{type_name}) UnmarshalJSON(data []byte) error {{
+ type Alias {type_name}
+ peek := struct {{
+ Alias
+ Driver string `json:"{variants.tag_member.name}"`
+ }}{{}}
+
+
+ if err := json.Unmarshal(data, &peek); err != nil {{
+ return err
+ }}
+ *s = {type_name}(peek.Alias)
+
+ switch peek.Driver {{
+ {driverCases}
+ }}
+ // Unrecognizer drivers are silently ignored.
+ return nil
+}}
+'''
+
+
def qapi_schema_type_to_go_type(type: str) -> str:
schema_types_to_go = {
'str': 'string', 'null': 'nil', 'bool': 'bool', 'number':
@@ -345,6 +439,10 @@ def qapi_to_field_name(name: str) -> str:
return name.title().replace("_", "").replace("-", "")
+def qapi_name_is_base(name: str) -> bool:
+ return name.startswith("q_obj_") and name.endswith("-base")
+
+
def qapi_to_go_type_name(name: str, meta: str) -> str:
if name.startswith("q_obj_"):
name = name[6:]
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (3 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union " Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-06-17 12:19 ` [RFC PATCH v2 6/8] qapi: golang: Generate qapi's command " Victor Toso
` (4 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
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.
At the moment of this writing, this patch generates 51 structures (50
events)
Example:
qapi:
| { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
| 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
go:
| type MemoryDeviceSizeChangeEvent struct {
| EventTimestamp 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 | 120 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 6c6a5cea97..b2e08cebdf 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -28,7 +28,66 @@
)
from .source import QAPISourceInfo
+# Only variable is @unm_cases to handle
+# all events's names and associated types.
+TEMPLATE_EVENT = '''
+type Timestamp struct {{
+ Seconds int64 `json:"seconds"`
+ Microseconds int64 `json:"microseconds"`
+}}
+
+type Event interface {{
+ GetName() string
+ GetTimestamp() Timestamp
+}}
+func MarshalEvent(e Event) ([]byte, error) {{
+ baseStruct := struct {{
+ Name string `json:"event"`
+ EventTimestamp Timestamp `json:"timestamp"`
+ }}{{
+ Name: e.GetName(),
+ EventTimestamp: e.GetTimestamp(),
+ }}
+ base, err := json.Marshal(baseStruct)
+ if err != nil {{
+ return []byte{{}}, err
+ }}
+
+ dataStruct := struct {{
+ Payload Event `json:"data"`
+ }}{{
+ Payload: e,
+ }}
+ data, err := json.Marshal(dataStruct)
+ if err != nil {{
+ return []byte{{}}, err
+ }}
+
+ if len(data) == len(`{{"data":{{}}}}`) {{
+ return base, nil
+ }}
+
+ // Combines Event's base and data in a single JSON object
+ result := fmt.Sprintf("%s,%s", base[:len(base)-1], data[1:])
+ return []byte(result), nil
+}}
+
+func UnmarshalEvent(data []byte) (Event, error) {{
+ base := struct {{
+ Name string `json:"event"`
+ EventTimestamp Timestamp `json:"timestamp"`
+ }}{{}}
+ if err := json.Unmarshal(data, &base); err != nil {{
+ return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
+ }}
+
+ switch base.Name {{
+ {unm_cases}
+ }}
+ return nil, errors.New("Failed to recognize event")
+}}
+'''
TEMPLATE_HELPER = '''
// Alias for go version lower than 1.18
type Any = interface{}
@@ -53,10 +112,12 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
def __init__(self, prefix: str):
super().__init__()
- self.target = {name: "" for name in ["alternate", "enum", "helper", "struct",
+ self.target = {name: "" for name in ["alternate", "enum",
+ "event", "helper", "struct",
"union"]}
self.objects_seen = {}
self.schema = None
+ self.events = {}
self.golang_package_name = "qapi"
def visit_begin(self, schema):
@@ -71,6 +132,24 @@ def visit_begin(self, schema):
def visit_end(self):
self.schema = None
+ unm_cases = ""
+ for name in sorted(self.events):
+ case_type = self.events[name]
+ unm_cases += f'''
+ case "{name}":
+ event := struct {{
+ Data {case_type} `json:"data"`
+ }}{{}}
+
+ if err := json.Unmarshal(data, &event); err != nil {{
+ return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
+ }}
+ event.Data.EventTimestamp = base.EventTimestamp
+ return &event.Data, nil
+'''
+ self.target["event"] += TEMPLATE_EVENT.format(unm_cases=unm_cases)
+
+
def visit_object_type(self: QAPISchemaGenGolangVisitor,
name: str,
info: Optional[QAPISourceInfo],
@@ -232,7 +311,37 @@ def visit_command(self,
pass
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
- pass
+ assert name == info.defn_name
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+ self.events[name] = type_name
+
+ self_contained = True
+ if arg_type and arg_type.name.startswith("q_obj"):
+ self_contained = False
+
+ content = ""
+ if self_contained:
+ content = generate_struct_type(type_name, '''EventTimestamp Timestamp `json:"-"`''')
+ else:
+ assert isinstance(arg_type, QAPISchemaObjectType)
+ content = qapi_to_golang_struct(name,
+ arg_type.info,
+ arg_type.ifcond,
+ arg_type.features,
+ arg_type.base,
+ arg_type.members,
+ arg_type.variants)
+
+ methods = f'''
+func (s *{type_name}) GetName() string {{
+ return "{name}"
+}}
+
+func (s *{type_name}) GetTimestamp() Timestamp {{
+ return s.EventTimestamp
+}}
+'''
+ self.target["event"] += content + methods
def write(self, output_dir: str) -> None:
for module_name, content in self.target.items():
@@ -274,6 +383,8 @@ def qapi_to_golang_struct(name: str,
type_name = qapi_to_go_type_name(name, info.defn_meta)
fields = ""
+ if info.defn_meta == "event":
+ fields += '''\tEventTimestamp Timestamp `json:"-"`\n'''
if base:
base_fields = ""
@@ -457,4 +568,9 @@ def qapi_to_go_type_name(name: str, meta: str) -> str:
name = name.title()
name += ''.join(word.title() for word in words[1:])
+
+ if meta in ["event"]:
+ name = name[:-3] if name.endswith("Arg") else name
+ name += meta.title().replace(" ", "")
+
return name
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 6/8] qapi: golang: Generate qapi's command types in Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (4 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event " Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go Victor Toso
` (3 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
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.
Simlar 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
| CommandId 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 | 123 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 120 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index b2e08cebdf..123179cced 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -88,6 +88,63 @@
return nil, errors.New("Failed to recognize event")
}}
'''
+
+# Only variable is @unm_cases to handle all command's names and associated types.
+TEMPLATE_COMMAND = '''
+type Command interface {{
+ GetId() string
+ GetName() string
+ GetReturnType() CommandReturn
+}}
+
+func MarshalCommand(c Command) ([]byte, error) {{
+ baseStruct := struct {{
+ CommandId string `json:"id,omitempty"`
+ Name string `json:"execute"`
+ }}{{
+ CommandId: c.GetId(),
+ Name: c.GetName(),
+ }}
+ base, err := json.Marshal(baseStruct)
+ if err != nil {{
+ return []byte{{}}, err
+ }}
+
+ argsStruct := struct {{
+ Args Command `json:"arguments,omitempty"`
+ }}{{
+ Args: c,
+ }}
+ args, err := json.Marshal(argsStruct)
+ if err != nil {{
+ return []byte{{}}, err
+ }}
+
+ if len(args) == len(`{{"arguments":{{}}}}`) {{
+ return base, nil
+ }}
+
+ // Combines Event's base and data in a single JSON object
+ result := fmt.Sprintf("%s,%s", base[:len(base)-1], args[1:])
+ return []byte(result), nil
+}}
+
+func UnmarshalCommand(data []byte) (Command, error) {{
+ base := struct {{
+ CommandId string `json:"id,omitempty"`
+ Name string `json:"execute"`
+ }}{{}}
+ if err := json.Unmarshal(data, &base); err != nil {{
+ return nil, errors.New(fmt.Sprintf("Failed to decode command: %s", string(data)))
+ }}
+
+ switch base.Name {{
+ {unm_cases}
+ }}
+ return nil, errors.New("Failed to recognize command")
+}}
+'''
+
TEMPLATE_HELPER = '''
// Alias for go version lower than 1.18
type Any = interface{}
@@ -112,12 +169,13 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
def __init__(self, prefix: str):
super().__init__()
- self.target = {name: "" for name in ["alternate", "enum",
+ self.target = {name: "" for name in ["alternate", "command", "enum",
"event", "helper", "struct",
"union"]}
self.objects_seen = {}
self.schema = None
self.events = {}
+ self.commands = {}
self.golang_package_name = "qapi"
def visit_begin(self, schema):
@@ -149,6 +207,23 @@ def visit_end(self):
'''
self.target["event"] += TEMPLATE_EVENT.format(unm_cases=unm_cases)
+ unm_cases = ""
+ for name in sorted(self.commands):
+ case_type = self.commands[name]
+ unm_cases += f'''
+ case "{name}":
+ command := struct {{
+ Args {case_type} `json:"arguments"`
+ }}{{}}
+
+ if err := json.Unmarshal(data, &command); err != nil {{
+ return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
+ }}
+ command.Args.CommandId = base.CommandId
+ return &command.Args, nil
+'''
+ self.target["command"] += TEMPLATE_COMMAND.format(unm_cases=unm_cases)
+
def visit_object_type(self: QAPISchemaGenGolangVisitor,
name: str,
@@ -308,7 +383,47 @@ def visit_command(self,
allow_oob: bool,
allow_preconfig: bool,
coroutine: bool) -> None:
- pass
+ # Safety check
+ assert name == info.defn_name
+
+ type_name = qapi_to_go_type_name(name, info.defn_meta)
+ self.commands[name] = type_name
+ command_ret = ""
+ init_ret_type_name = f'''EmptyCommandReturn {{ Name: "{name}" }}'''
+
+ self_contained = True
+ if arg_type and arg_type.name.startswith("q_obj"):
+ self_contained = False
+
+ content = ""
+ if boxed or self_contained:
+ args = "" if not arg_type else "\n" + arg_type.name
+ args += '''\n\tCommandId string `json:"-"`'''
+ content = generate_struct_type(type_name, args)
+ else:
+ assert isinstance(arg_type, QAPISchemaObjectType)
+ content = qapi_to_golang_struct(name,
+ arg_type.info,
+ arg_type.ifcond,
+ arg_type.features,
+ arg_type.base,
+ arg_type.members,
+ arg_type.variants)
+
+ methods = f'''
+func (c *{type_name}) GetName() string {{
+ return "{name}"
+}}
+
+func (s *{type_name}) GetId() string {{
+ return s.CommandId
+}}
+
+func (s *{type_name}) GetReturnType() CommandReturn {{
+ return &{init_ret_type_name}
+}}
+'''
+ self.target["command"] += content + methods
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
assert name == info.defn_name
@@ -385,6 +500,8 @@ def qapi_to_golang_struct(name: str,
fields = ""
if info.defn_meta == "event":
fields += '''\tEventTimestamp Timestamp `json:"-"`\n'''
+ elif info.defn_meta == "command":
+ fields += '''\tCommandId string `json:"-"`\n'''
if base:
base_fields = ""
@@ -569,7 +686,7 @@ def qapi_to_go_type_name(name: str, meta: str) -> str:
name += ''.join(word.title() for word in words[1:])
- if meta in ["event"]:
+ if meta in ["event", "command"]:
name = name[:-3] if name.endswith("Arg") else name
name += meta.title().replace(" ", "")
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (5 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 6/8] qapi: golang: Generate qapi's command " Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-07-05 15:46 ` Andrea Bolognani
2022-06-17 12:19 ` [RFC PATCH v2 8/8] qapi: golang: document skip function visit_array_types Victor Toso
` (2 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
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 {
| CommandId 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 | 73 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 70 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index 123179cced..ab91cf124f 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -89,7 +89,8 @@
}}
'''
-# Only variable is @unm_cases to handle all command's names and associated types.
+# Only variable is @unm_cases to handle
+# all command's names and associated types.
TEMPLATE_COMMAND = '''
type Command interface {{
GetId() string
@@ -145,10 +146,49 @@
}}
'''
+TEMPLATE_COMMAND_RETURN = '''
+type CommandReturn interface {
+ GetId() string
+ GetCommandName() string
+ GetError() error
+}
+
+type EmptyCommandReturn struct {
+ CommandId string `json:"id,omitempty"`
+ Error *QapiError `json:"error,omitempty"`
+ Name string `json:"-"`
+}
+
+func (r EmptyCommandReturn) MarshalJSON() ([]byte, error) {
+ return []byte(`{"return":{}}`), nil
+}
+
+func (r *EmptyCommandReturn) GetId() string {
+ return r.CommandId
+}
+
+func (r *EmptyCommandReturn) GetCommandName() string {
+ return r.Name
+}
+
+func (r *EmptyCommandReturn) GetError() error {
+ return r.Error
+}
+'''
+
TEMPLATE_HELPER = '''
// Alias for go version lower than 1.18
type Any = interface{}
+type QapiError struct {
+ Class string `json:"class"`
+ Description string `json:"desc"`
+}
+
+func (err *QapiError) Error() string {
+ return fmt.Sprintf("%s: %s", err.Class, err.Description)
+}
+
// Creates a decoder that errors on unknown Fields
// Returns true if successfully decoded @from string @into type
// Returns false without error is failed with "unknown field"
@@ -176,6 +216,7 @@ def __init__(self, prefix: str):
self.schema = None
self.events = {}
self.commands = {}
+ self.command_results = {}
self.golang_package_name = "qapi"
def visit_begin(self, schema):
@@ -224,6 +265,7 @@ def visit_end(self):
'''
self.target["command"] += TEMPLATE_COMMAND.format(unm_cases=unm_cases)
+ self.target["command"] += TEMPLATE_COMMAND_RETURN
def visit_object_type(self: QAPISchemaGenGolangVisitor,
name: str,
@@ -390,6 +432,31 @@ def visit_command(self,
self.commands[name] = type_name
command_ret = ""
init_ret_type_name = f'''EmptyCommandReturn {{ Name: "{name}" }}'''
+ if ret_type:
+ cmd_ret_name = qapi_to_go_type_name(name, "command return")
+ ret_type_name = qapi_schema_type_to_go_type(ret_type.name)
+ init_ret_type_name = f'''{cmd_ret_name}{{}}'''
+ isptr = "*" if ret_type_name[0] not in "*[" else ""
+ self.command_results[name] = ret_type_name
+ command_ret = f'''
+type {cmd_ret_name} struct {{
+ CommandId string `json:"id,omitempty"`
+ Result {isptr}{ret_type_name} `json:"return"`
+ Error *QapiError `json:"error,omitempty"`
+}}
+
+func (r *{cmd_ret_name}) GetCommandName() string {{
+ return "{name}"
+}}
+
+func (r *{cmd_ret_name}) GetId() string {{
+ return r.CommandId
+}}
+
+func (r *{cmd_ret_name}) GetError() error {{
+ return r.Error
+}}
+'''
self_contained = True
if arg_type and arg_type.name.startswith("q_obj"):
@@ -423,7 +490,7 @@ def visit_command(self,
return &{init_ret_type_name}
}}
'''
- self.target["command"] += content + methods
+ self.target["command"] += content + methods + command_ret
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
assert name == info.defn_name
@@ -686,7 +753,7 @@ def qapi_to_go_type_name(name: str, meta: str) -> str:
name += ''.join(word.title() for word in words[1:])
- if meta in ["event", "command"]:
+ if meta in ["event", "command", "command return"]:
name = name[:-3] if name.endswith("Arg") else name
name += meta.title().replace(" ", "")
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [RFC PATCH v2 8/8] qapi: golang: document skip function visit_array_types
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (6 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go Victor Toso
@ 2022-06-17 12:19 ` Victor Toso
2022-06-27 7:15 ` [RFC PATCH v2 0/8] qapi: add generator for Golang interface Markus Armbruster
2022-07-05 15:46 ` Andrea Bolognani
9 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 12:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
scripts/qapi/golang.py | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
index ab91cf124f..f37014f52b 100644
--- a/scripts/qapi/golang.py
+++ b/scripts/qapi/golang.py
@@ -410,7 +410,12 @@ def visit_enum_type(self: QAPISchemaGenGolangVisitor,
'''
def visit_array_type(self, name, info, ifcond, element_type):
- pass
+ # TLDR: We don't need to any extra boilerplate in Go to handle Arrays.
+ #
+ # This function is implemented just to be sure that:
+ # 1. Every array type ends with List
+ # 2. Every array type's element is the array type without 'List'
+ assert name.endswith("List") and name[:-4] == element_type.name
def visit_command(self,
name: str,
--
2.36.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct types in Go
2022-06-17 12:19 ` [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct " Victor Toso
@ 2022-06-17 14:41 ` Daniel P. Berrangé
2022-06-17 15:23 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-06-17 14:41 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani
On Fri, Jun 17, 2022 at 02:19:27PM +0200, Victor Toso wrote:
> This patch handles QAPI struct types and generates the equivalent
> types in Go.
>
> At the time of this writing, it generates 388 structures.
>
> 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.
> The downside are some generated Types that are likely useless now,
> like InetSocketAddressBase from InetSocketAddress.
>
> 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.
>
> 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 ordecoding, 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"`
> | }
One thing to bear in mind here
At the QAPI level, changing a field from mandatory to optional has
been considered a backwards compatible change by QEMU maintainers,
because any existing caller can happily continue passing the
optional field with no downside.
With this Go design, changing a field from mandatory to optional
will be an API breakage, because the developer will need to change
from passing a literal value, to a pointer to the value, when
initializing the struct.
IOW, this Go impl provides weaker compat guarantees than even
QAPI does, and QAPI compat guarantees were already weaker than
I would like as an app developer.
If we want to make ourselves future proof, we would have to make
all struct fields optional from the start, even if they are
mandatory at QAPI level. This would make the code less self-documenting
though, so that's not very appealing either.
If we want to avoid this, we would need the same approach I suggested
wrt support multiple versions of the API concurrently. Namely have
versioned structs, so every time there's a field change of any kind,
we introduce a new struct version.
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] 47+ messages in thread
* Re: [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct types in Go
2022-06-17 14:41 ` Daniel P. Berrangé
@ 2022-06-17 15:23 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-06-17 15:23 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani
[-- Attachment #1: Type: text/plain, Size: 4126 bytes --]
Hi,
On Fri, Jun 17, 2022 at 03:41:10PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 17, 2022 at 02:19:27PM +0200, Victor Toso wrote:
> > This patch handles QAPI struct types and generates the equivalent
> > types in Go.
> >
> > At the time of this writing, it generates 388 structures.
> >
> > 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.
> > The downside are some generated Types that are likely useless now,
> > like InetSocketAddressBase from InetSocketAddress.
> >
> > 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.
> >
> > 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 ordecoding, 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"`
> > | }
>
> One thing to bear in mind here
>
> At the QAPI level, changing a field from mandatory to optional has
> been considered a backwards compatible change by QEMU maintainers,
> because any existing caller can happily continue passing the
> optional field with no downside.
>
> With this Go design, changing a field from mandatory to optional
> will be an API breakage, because the developer will need to change
> from passing a literal value, to a pointer to the value, when
> initializing the struct.
>
> IOW, this Go impl provides weaker compat guarantees than even
> QAPI does, and QAPI compat guarantees were already weaker than
> I would like as an app developer.
I think the current draft should be considered an interface that
can work with the QEMU version this was generated from. That is
the first thing we should get right.
> If we want to make ourselves future proof, we would have to
> make all struct fields optional from the start, even if they
> are mandatory at QAPI level. This would make the code less
> self-documenting though, so that's not very appealing either.
> If we want to avoid this, we would need the same approach I
> suggested wrt support multiple versions of the API
> concurrently. Namely have versioned structs, so every time
> there's a field change of any kind, we introduce a new struct
> version.
That's more or less what I had in mind. I mentioned it in the
item 8 of the cover-letter. I just did not want to address it at
before deciding what the structs should look like first, for the
version we are generating from.
Just to clarify, so far I plan to follow the suggestion:
https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg02147.html
Of course, If there are other ideas, we can discuss it too.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (7 preceding siblings ...)
2022-06-17 12:19 ` [RFC PATCH v2 8/8] qapi: golang: document skip function visit_array_types Victor Toso
@ 2022-06-27 7:15 ` Markus Armbruster
2022-06-27 12:48 ` Victor Toso
2022-07-05 15:46 ` Andrea Bolognani
9 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2022-06-27 7:15 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, John Snow, Andrea Bolognani,
Daniel P . Berrangé
Victor Toso <victortoso@redhat.com> writes:
> Hi,
>
> This is the second iteration of RFC v1:
> https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html
>
>
> # What this is about?
>
> To generate a simple Golang interface that could communicate with QEMU
> over QMP. The Go code that is generated is meant to be used as the bare
> bones to exchange QMP messages.
>
> The goal is to have this as a Go module in QEMU gitlab namespace,
> similar to what have been done to pyhon-qemu-qmp
> https://gitlab.com/qemu-project/python-qemu-qmp
Aspects of review:
(1) Impact on common code, if any
I care, because any messes made there are likely to affect me down
the road.
(2) The generated Go code
Is it (close to) what we want long term? If not, is it good enough
short term, and how could we make necessary improvements?
I'd prefer to leave this to folks who actually know their Go.
(3) General Python sanity
We need eyes, but not necessarily mine. Any takers?
[...]
> scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++
> scripts/qapi/main.py | 2 +
> 2 files changed, 767 insertions(+)
> create mode 100644 scripts/qapi/golang.py
This adds a new generator and calls it from generate(), i.e. review
aspect (1) is empty. "Empty" is a quick & easy way to get my ACK!
No tests?
No documentation?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-06-27 7:15 ` [RFC PATCH v2 0/8] qapi: add generator for Golang interface Markus Armbruster
@ 2022-06-27 12:48 ` Victor Toso
2022-06-27 15:29 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-06-27 12:48 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, John Snow, Andrea Bolognani,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]
Hi Markus,
On Mon, Jun 27, 2022 at 09:15:53AM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > This is the second iteration of RFC v1:
> > https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html
> >
> >
> > # What this is about?
> >
> > To generate a simple Golang interface that could communicate with QEMU
> > over QMP. The Go code that is generated is meant to be used as the bare
> > bones to exchange QMP messages.
> >
> > The goal is to have this as a Go module in QEMU gitlab namespace,
> > similar to what have been done to pyhon-qemu-qmp
> > https://gitlab.com/qemu-project/python-qemu-qmp
>
> Aspects of review:
>
> (1) Impact on common code, if any
>
> I care, because any messes made there are likely to affect me down
> the road.
For the first version of the Go generated interface, my goal is
to have something that works and can be considered alpha to other
Go projects.
With this first version, I don't want to bring huge changes to
the python library or to the QAPI spec and its usage in QEMU.
Unless someone finds that a necessity.
So I hope (1) is simple :)
> (2) The generated Go code
>
> Is it (close to) what we want long term? If not, is it good enough
> short term, and how could we make necessary improvements?
>
> I'd prefer to leave this to folks who actually know their Go.
> (3) General Python sanity
>
> We need eyes, but not necessarily mine. Any takers?
>
> [...]
>
> > scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++
> > scripts/qapi/main.py | 2 +
> > 2 files changed, 767 insertions(+)
> > create mode 100644 scripts/qapi/golang.py
>
> This adds a new generator and calls it from generate(), i.e.
> review aspect (1) is empty. "Empty" is a quick & easy way to
> get my ACK!
>
> No tests?
I've added tests but on the qapi-go module, those are the files
with _test.go prefix on them. Example for commands:
https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/commands_test.go
Should the generator itself have tests or offloading that to the
qapi-go seems reasonable?
> No documentation?
Yes, this iteration removed the documentation of the generated
types. I'm a bit sad about that. I want to consume the
documentation in the QAPI files to provide the latest info from
types/fields but we can't 'copy' it, the only solution is 'refer'
to it with hyperlink, which I haven't done yet.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-06-27 12:48 ` Victor Toso
@ 2022-06-27 15:29 ` Markus Armbruster
2022-08-18 8:04 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2022-06-27 15:29 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, John Snow, Andrea Bolognani,
Daniel P . Berrangé
Victor Toso <victortoso@redhat.com> writes:
> Hi Markus,
>
> On Mon, Jun 27, 2022 at 09:15:53AM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > Hi,
>> >
>> > This is the second iteration of RFC v1:
>> > https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html
>> >
>> >
>> > # What this is about?
>> >
>> > To generate a simple Golang interface that could communicate with QEMU
>> > over QMP. The Go code that is generated is meant to be used as the bare
>> > bones to exchange QMP messages.
>> >
>> > The goal is to have this as a Go module in QEMU gitlab namespace,
>> > similar to what have been done to pyhon-qemu-qmp
>> > https://gitlab.com/qemu-project/python-qemu-qmp
>>
>> Aspects of review:
>>
>> (1) Impact on common code, if any
>>
>> I care, because any messes made there are likely to affect me down
>> the road.
>
> For the first version of the Go generated interface, my goal is
> to have something that works and can be considered alpha to other
> Go projects.
>
> With this first version, I don't want to bring huge changes to
> the python library or to the QAPI spec and its usage in QEMU.
> Unless someone finds that a necessity.
>
> So I hope (1) is simple :)
>
>> (2) The generated Go code
>>
>> Is it (close to) what we want long term? If not, is it good enough
>> short term, and how could we make necessary improvements?
>>
>> I'd prefer to leave this to folks who actually know their Go.
>> (3) General Python sanity
>>
>> We need eyes, but not necessarily mine. Any takers?
>>
>> [...]
>>
>> > scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++
>> > scripts/qapi/main.py | 2 +
>> > 2 files changed, 767 insertions(+)
>> > create mode 100644 scripts/qapi/golang.py
>>
>> This adds a new generator and calls it from generate(), i.e.
>> review aspect (1) is empty. "Empty" is a quick & easy way to
>> get my ACK!
>>
>> No tests?
>
> I've added tests but on the qapi-go module, those are the files
> with _test.go prefix on them. Example for commands:
>
> https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/commands_test.go
>
> Should the generator itself have tests or offloading that to the
> qapi-go seems reasonable?
Offloading may be reasonable, but how am I to run the tests then?
Documentation should tell me.
We have roughly three kinds of tests so far:
1. Front end tests in tests/qapi-schema
2. Unit tests in tests/unit/
To find them:
$ git-grep '#include ".*qapi-.*\.h"' tests/unit/
3. Many tests talking QMP in tests/qtest/
Since you leave the front end alone, you don't need the first kind.
The other two kinds are less clear.
>> No documentation?
>
> Yes, this iteration removed the documentation of the generated
> types. I'm a bit sad about that. I want to consume the
> documentation in the QAPI files to provide the latest info from
> types/fields but we can't 'copy' it, the only solution is 'refer'
> to it with hyperlink, which I haven't done yet.
Two kinds of documentation: generated documentation for the generated Go
code, and documentation about the generator. I was thinking of the
latter. Specifically, docs/devel/qapi-code-gen.rst section "Code
generation". Opinions on its usefulness differ. I like it.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-06-17 12:19 ` [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate " Victor Toso
@ 2022-07-05 15:45 ` Andrea Bolognani
2022-08-17 14:04 ` Victor Toso
2022-09-02 14:49 ` Victor Toso
1 sibling, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-05 15:45 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
Sorry it took me a while to find the time to look into this!
Overall this second iteration is a significant improvement over the
initial one. There are still a few things that I think should be
changed, so for the time being I'm going to comment mostly on the
generated Go code and leave the details of the implementation for
later.
On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> At this moment, there are 5 alternates in qemu/qapi, they are:
> * BlockDirtyBitmapMergeSource
> * Qcow2OverlapChecks
> * BlockdevRef
> * BlockdevRefOrNull
> * StrOrNull
I personally don't think it's very useful to list all the alternate
types in the commit message, or even mention how many there are. You
do this for all other types too, and it seems to me that it's just an
opportunity for incosistent information to make their way to the git
repository - what if a new type is introduced between the time your
series is queued and merged? I'd say just drop this part.
> 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"
Let me just say that I absolutely *love* how you've added these bits
comparing the QAPI / Go representations as well as usage. They really
help a lot understanding what the generator is trying to achieve :)
> +// Creates a decoder that errors on unknown Fields
> +// Returns true if successfully decoded @from string @into type
> +// Returns false without error is failed with "unknown field"
> +// Returns false with error is a different error was found
> +func StrictDecode(into interface{}, from []byte) error {
> + dec := json.NewDecoder(strings.NewReader(string(from)))
> + dec.DisallowUnknownFields()
> +
> + if err := dec.Decode(into); err != nil {
> + return err
> + }
> + return nil
> +}
The documentation doesn't seem to be consistent with how the function
actually works: AFAICT it returns false *with an error* for any
failure, including those caused by unknown fields being present in
the input JSON.
Looking at the generated code:
> type BlockdevRef struct {
> Definition *BlockdevOptions
> Reference *string
> }
>
> func (s BlockdevRef) MarshalJSON() ([]byte, error) {
> if s.Definition != nil {
> return json.Marshal(s.Definition)
> } else if s.Reference != nil {
> return json.Marshal(s.Reference)
> }
> return nil, errors.New("BlockdevRef has empty fields")
Returning an error if no field is set is good. Can we be more strict
and returning one if more than one is set as well? That feels better
than just picking the first one.
> func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> // Check for json-null first
> if string(data) == "null" {
> return errors.New(`null not supported for BlockdevRef`)
> }
> // Check for BlockdevOptions
> {
> s.Definition = new(BlockdevOptions)
> if err := StrictDecode(s.Definition, data); err == nil {
> return nil
> }
The use of StrictDecode() here means that we won't be able to parse
an alternate produced by a version of QEMU where BlockdevOptions has
gained additional fields, doesn't it?
Considering that we will happily parse such a BlockdevOptions outside
of the context of BlockdevRef, I think we should be consistent and
allow the same to happen here.
> s.Definition = nil
> }
> // Check for string
> {
> s.Reference = new(string)
> if err := StrictDecode(s.Reference, data); err == nil {
> return nil
> }
> s.Reference = nil
> }
>
> return errors.New(fmt.Sprintf("Can't convert to BlockdevRef: %s", string(data)))
On a similar note to the MarshalJSON comment above, I'm not sure this
is right.
If we find that more than one field of the alternate is set, we
should error out - that's just invalid JSON we're dealing with. But
if we couldn't find any, that might be valid JSON that's been
produced by a version of QEMU that introduced additional options to
the alternate. In the spirit of "be liberal in what you accept", I
think we should probably not reject that? Of course then the client
code will have to look like
if r.Definition != nil {
// do something with r.Definition
} else if r.Reference != nil {
// do something with r.Reference
} else {
// we don't recognize this - error out
}
but the same is going to be true for enums, events and everything
else that can be extended in a backwards-compatible fashion.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-06-17 12:19 ` [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union " Victor Toso
@ 2022-07-05 15:45 ` Andrea Bolognani
2022-07-05 16:35 ` Daniel P. Berrangé
0 siblings, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-05 15:45 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Fri, Jun 17, 2022 at 02:19:28PM +0200, Victor Toso wrote:
> qapi:
> | { 'union': 'SetPasswordOptions',
> | 'base': { 'protocol': 'DisplayProtocol',
> | 'password': 'str',
> | '*connected': 'SetPasswordAction' },
> | 'discriminator': 'protocol',
> | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
>
> go:
> | type SetPasswordOptions struct {
> | // Base fields
> | Password string `json:"password"`
> | Connected *SetPasswordAction `json:"connected,omitempty"`
> |
> | // Variants fields
> | Vnc *SetPasswordOptionsVnc `json:"-"`
> | }
Generated code:
> type GuestPanicInformation struct {
> // Variants fields
> HyperV *GuestPanicInformationHyperV `json:"-"`
> S390 *GuestPanicInformationS390 `json:"-"`
> }
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> type Alias GuestPanicInformation
> alias := Alias(s)
> base, err := json.Marshal(alias)
> if err != nil {
> return nil, err
> }
>
> driver := ""
> payload := []byte{}
> if s.HyperV != nil {
> driver = "hyper-v"
> payload, err = json.Marshal(s.HyperV)
> } else if s.S390 != nil {
> driver = "s390"
> payload, err = json.Marshal(s.S390)
> }
>
> if err != nil {
> return nil, err
> }
>
> if len(base) == len("{}") {
> base = []byte("")
> } else {
> base = append([]byte{','}, base[1:len(base)-1]...)
> }
>
> if len(payload) == 0 || len(payload) == len("{}") {
> payload = []byte("")
> } else {
> payload = append([]byte{','}, payload[1:len(payload)-1]...)
> }
>
> result := fmt.Sprintf(`{"type":"%s"%s%s}`, driver, base, payload)
> return []byte(result), nil
All this string manipulation looks sketchy. Is there some reason that
I'm not seeing preventing you for doing something like the untested
code below?
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
if s.HyperV != nil {
type union struct {
Discriminator string `json:"type"`
HyperV GuestPanicInformationHyperV `json:"hyper-v"`
}
tmp := union {
Discriminator: "hyper-v",
HyperV: s.HyperV,
}
return json.Marshal(tmp)
} else if s.S390 != nil {
type union struct {
Discriminator string `json:"type"`
S390 GuestPanicInformationHyperV `json:"s390"`
}
tmp := union {
Discriminator: "s390",
S390: s.S390,
}
return json.Marshal(tmp)
}
return nil, errors.New("...")
}
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> type Alias GuestPanicInformation
> peek := struct {
> Alias
> Driver string `json:"type"`
> }{}
>
> if err := json.Unmarshal(data, &peek); err != nil {
> return err
> }
> *s = GuestPanicInformation(peek.Alias)
>
> switch peek.Driver {
>
> case "hyper-v":
> s.HyperV = new(GuestPanicInformationHyperV)
> if err := json.Unmarshal(data, s.HyperV); err != nil {
> s.HyperV = nil
> return err
> }
> case "s390":
> s.S390 = new(GuestPanicInformationS390)
> if err := json.Unmarshal(data, s.S390); err != nil {
> s.S390 = nil
> return err
> }
> }
> // Unrecognizer drivers are silently ignored.
> return nil
This looks pretty reasonable, but since you're only using "peek" to
look at the discriminator you should be able to leave out the Alias
type entirely and perform the initial Unmarshal operation while
ignoring all other fields.
The comments made elsewhere about potentially being more strict and
rejecting invalid JSON also apply here.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-06-17 12:19 ` [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event " Victor Toso
@ 2022-07-05 15:45 ` Andrea Bolognani
2022-07-05 16:47 ` Daniel P. Berrangé
2022-08-18 7:47 ` Victor Toso
0 siblings, 2 replies; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-05 15:45 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Fri, Jun 17, 2022 at 02:19:29PM +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.
>
> At the moment of this writing, this patch generates 51 structures (50
> events)
>
> Example:
>
> qapi:
> | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>
> go:
> | type MemoryDeviceSizeChangeEvent struct {
> | EventTimestamp 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]"
> | }
Generated code:
> type AcpiDeviceOstEvent struct {
> EventTimestamp Timestamp `json:"-"`
Any reason for naming this EventTimestamp as opposed to just
Timestamp?
> Info ACPIOSTInfo `json:"info"`
> }
>
> func (s *AcpiDeviceOstEvent) GetName() string {
> return "ACPI_DEVICE_OST"
> }
Getters in Go don't usually start with "Get", so this could be just
Name(). And pointer receivers are usually only recommended when the
underlying object needs to be modified, which is not the case here.
> func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> return s.EventTimestamp
> }
Does this even need a getter? The struct member is public, and Go
code seems to favor direct member access.
> type Timestamp struct {
> Seconds int64 `json:"seconds"`
> Microseconds int64 `json:"microseconds"`
> }
>
> type Event interface {
> GetName() string
> GetTimestamp() Timestamp
> }
>
> func MarshalEvent(e Event) ([]byte, error) {
> baseStruct := struct {
> Name string `json:"event"`
> EventTimestamp Timestamp `json:"timestamp"`
> }{
> Name: e.GetName(),
> EventTimestamp: e.GetTimestamp(),
> }
> base, err := json.Marshal(baseStruct)
> if err != nil {
> return []byte{}, err
> }
>
> dataStruct := struct {
> Payload Event `json:"data"`
> }{
> Payload: e,
> }
> data, err := json.Marshal(dataStruct)
> if err != nil {
> return []byte{}, err
> }
>
> if len(data) == len(`{"data":{}}`) {
> return base, nil
> }
>
> // Combines Event's base and data in a single JSON object
> result := fmt.Sprintf("%s,%s", base[:len(base)-1], data[1:])
> return []byte(result), nil
> }
I have the same concerns about the string manipulation going on here
as I had for unions. Here's an alternative implementation, and this
time I've even tested it :)
func (s AcpiDeviceOstEvent) MarshalJSON() ([]byte, error) {
type eventData struct {
Info ACPIOSTInfo `json:"info"`
}
type event struct {
Name string `json:"event"`
Timestamp Timestamp `json:"timestamp"`
Data eventData `json:"data"`
}
tmp := event{
Name: "ACPI_DEVICE_OST",
Timestamp: s.EventTimestamp,
Data: eventData{
Info: s.Info,
},
}
return json.Marshal(tmp)
}
> func UnmarshalEvent(data []byte) (Event, error) {
> base := struct {
> Name string `json:"event"`
> EventTimestamp Timestamp `json:"timestamp"`
> }{}
> if err := json.Unmarshal(data, &base); err != nil {
> return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
> }
>
> switch base.Name {
>
> case "ACPI_DEVICE_OST":
> event := struct {
> Data AcpiDeviceOstEvent `json:"data"`
> }{}
>
> if err := json.Unmarshal(data, &event); err != nil {
> return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
> }
> event.Data.EventTimestamp = base.EventTimestamp
> return &event.Data, nil
>
> // more cases here
> }
> return nil, errors.New("Failed to recognize event")
> }
While I understand the need to have some way to figure out exactly
what type of event we're looking at before being able to unmarshal
the JSON data, I don't like how we force users to go through a
non-standard API for it.
Here's how I think we should do it:
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, errors.New(fmt.Sprintf("Failed to decode event:
%s", string(data)))
}
switch tmp.Name {
case "ACPI_DEVICE_OST":
return &AcpiDeviceOstEvent{}, nil
// more cases here
}
return nil, errors.New("Failed to recognize event")
}
func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
type eventData struct {
Info ACPIOSTInfo `json:"info"`
}
type event struct {
Name string `json:"event"`
EventTimestamp Timestamp `json:"timestamp"`
Data eventData `json:"data"`
}
tmp := event{}
err := json.Unmarshal(data, &tmp)
if err != nil {
return err
}
if tmp.Name != "ACPI_DEVICE_OST" {
return errors.New("name")
}
s.EventTimestamp = tmp.EventTimestamp
s.Info = tmp.Data.Info
return nil
}
This way, client code can look like
in := `{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
e, err := qapi.GetEventType([]byte(in))
if err != nil {
panic(err)
}
// e is of type AcpiDeviceOstEvent
err = json.Unmarshal([]byte(in), &e)
if err != nil {
panic(err)
}
where only the first part (figuring out the type of the event) needs
custom APIs, and the remaining code looks just like your average JSON
handling.
Speaking of client code, in the commit message you have
> 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 the use of string comparison for
the purpose of going from a generic Event instance to a specific one:
a better way would be to use Go's type switch feature, such as
switch m := e.(type) {
case *MemoryDeviceSizeChangeEvent:
// m.QomPath == "/machine/unattached/device[2]"
}
In fact, I don't really see a reason to provide the Name() getter
outside of possibly diagnostics, and given the potential of it being
misused I would argue that it might be better not to have it.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go
2022-06-17 12:19 ` [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go Victor Toso
@ 2022-07-05 15:46 ` Andrea Bolognani
2022-07-05 16:49 ` Daniel P. Berrangé
0 siblings, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-05 15:46 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Fri, Jun 17, 2022 at 02:19:31PM +0200, Victor Toso wrote:
> +type EmptyCommandReturn struct {
> + CommandId string `json:"id,omitempty"`
> + Error *QapiError `json:"error,omitempty"`
> + Name string `json:"-"`
> +}
Do we need a specific type for this? Can't we just generate an
appropriately-named type for each of the commands that don't return
any data? It's not like we would have to write that code manually :)
> +func (r *EmptyCommandReturn) GetCommandName() string {
> + return r.Name
> +}
Just like Event.GetName() and Command.GetName(), I'm not convinced we
should have this.
Of course, all the comments about how marshalling and unmarshalling
are handled made for events also apply here.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
` (8 preceding siblings ...)
2022-06-27 7:15 ` [RFC PATCH v2 0/8] qapi: add generator for Golang interface Markus Armbruster
@ 2022-07-05 15:46 ` Andrea Bolognani
2022-08-17 14:24 ` Victor Toso
9 siblings, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-05 15:46 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
I've commented in detail to the single patches, just a couple of
additional points.
On Fri, Jun 17, 2022 at 02:19:24PM +0200, Victor Toso wrote:
> * 7) Flat structs by removing embed types. Discussion with Andrea
> Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
>
> No one required it but I decided to give it a try. Major issue that
> I see with this approach is to have generated a few 'Base' structs
> that are now useless. Overall, less nested structs seems better to
> me. Opnions?
>
> Example:
> | /* This is now useless, should be removed? */
> | type InetSocketAddressBase struct {
> | Host string `json:"host"`
> | Port string `json:"port"`
> | }
Can we somehow keep track, in the generator, of types that are only
used as building blocks for other types, and prevent them from
showing up in the generated code?
Finally, looking at the repository containing the generated code I
see that the generated type are sorted by kind, e.g. all unions are
in a file, all events in another one and so on. I believe the
structure should match more closely that of the QAPI schema, so e.g.
block-related types should all go in one file, net-related types in
another one and so on.
Looking forward to the next iteration :)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-05 15:45 ` Andrea Bolognani
@ 2022-07-05 16:35 ` Daniel P. Berrangé
2022-07-06 9:28 ` Andrea Bolognani
0 siblings, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-05 16:35 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> On Fri, Jun 17, 2022 at 02:19:28PM +0200, Victor Toso wrote:
> > qapi:
> > | { 'union': 'SetPasswordOptions',
> > | 'base': { 'protocol': 'DisplayProtocol',
> > | 'password': 'str',
> > | '*connected': 'SetPasswordAction' },
> > | 'discriminator': 'protocol',
> > | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
> >
> > go:
> > | type SetPasswordOptions struct {
> > | // Base fields
> > | Password string `json:"password"`
> > | Connected *SetPasswordAction `json:"connected,omitempty"`
> > |
> > | // Variants fields
> > | Vnc *SetPasswordOptionsVnc `json:"-"`
> > | }
>
> Generated code:
>
> > type GuestPanicInformation struct {
> > // Variants fields
> > HyperV *GuestPanicInformationHyperV `json:"-"`
> > S390 *GuestPanicInformationS390 `json:"-"`
> > }
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > type Alias GuestPanicInformation
> > alias := Alias(s)
> > base, err := json.Marshal(alias)
> > if err != nil {
> > return nil, err
> > }
> >
> > driver := ""
> > payload := []byte{}
> > if s.HyperV != nil {
> > driver = "hyper-v"
> > payload, err = json.Marshal(s.HyperV)
> > } else if s.S390 != nil {
> > driver = "s390"
> > payload, err = json.Marshal(s.S390)
> > }
> >
> > if err != nil {
> > return nil, err
> > }
> >
> > if len(base) == len("{}") {
> > base = []byte("")
> > } else {
> > base = append([]byte{','}, base[1:len(base)-1]...)
> > }
> >
> > if len(payload) == 0 || len(payload) == len("{}") {
> > payload = []byte("")
> > } else {
> > payload = append([]byte{','}, payload[1:len(payload)-1]...)
> > }
> >
> > result := fmt.Sprintf(`{"type":"%s"%s%s}`, driver, base, payload)
> > return []byte(result), nil
>
> All this string manipulation looks sketchy. Is there some reason that
> I'm not seeing preventing you for doing something like the untested
> code below?
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> if s.HyperV != nil {
> type union struct {
> Discriminator string `json:"type"`
> HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> }
> tmp := union {
> Discriminator: "hyper-v",
> HyperV: s.HyperV,
> }
> return json.Marshal(tmp)
> } else if s.S390 != nil {
> type union struct {
> Discriminator string `json:"type"`
> S390 GuestPanicInformationHyperV `json:"s390"`
> }
> tmp := union {
> Discriminator: "s390",
> S390: s.S390,
> }
> return json.Marshal(tmp)
> }
> return nil, errors.New("...")
> }
Using these dummy structs is the way I've approached the
discriminated union issue in the libvirt Golang XML bindings
and it works well. It is the bit I like the least, but it was
the lesser of many evils, and on the plus side in the QEMU case
it'll be auto-generated code.
>
> > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > type Alias GuestPanicInformation
> > peek := struct {
> > Alias
> > Driver string `json:"type"`
> > }{}
> >
> > if err := json.Unmarshal(data, &peek); err != nil {
> > return err
> > }
> > *s = GuestPanicInformation(peek.Alias)
> >
> > switch peek.Driver {
> >
> > case "hyper-v":
> > s.HyperV = new(GuestPanicInformationHyperV)
> > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > s.HyperV = nil
> > return err
> > }
> > case "s390":
> > s.S390 = new(GuestPanicInformationS390)
> > if err := json.Unmarshal(data, s.S390); err != nil {
> > s.S390 = nil
> > return err
> > }
> > }
> > // Unrecognizer drivers are silently ignored.
> > return nil
>
> This looks pretty reasonable, but since you're only using "peek" to
> look at the discriminator you should be able to leave out the Alias
> type entirely and perform the initial Unmarshal operation while
> ignoring all other fields.
Once you've defined the dummy structs for the Marshall case
though, you might as well use them for Unmarshall too, so you're
not parsing the JSON twice.
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] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-05 15:45 ` Andrea Bolognani
@ 2022-07-05 16:47 ` Daniel P. Berrangé
2022-07-06 14:53 ` Andrea Bolognani
2022-08-18 7:55 ` Victor Toso
2022-08-18 7:47 ` Victor Toso
1 sibling, 2 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-05 16:47 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> On Fri, Jun 17, 2022 at 02:19:29PM +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.
> >
> > At the moment of this writing, this patch generates 51 structures (50
> > events)
> >
> > Example:
> >
> > qapi:
> > | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> > | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
> >
> > go:
> > | type MemoryDeviceSizeChangeEvent struct {
> > | EventTimestamp 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]"
> > | }
> > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > return s.EventTimestamp
> > }
>
> Does this even need a getter? The struct member is public, and Go
> code seems to favor direct member access.
It satisfies the 'Event' interface, so you can fetch timestamp
without needing to know the specific event struct you're dealing
with.
>
> > type Timestamp struct {
> > Seconds int64 `json:"seconds"`
> > Microseconds int64 `json:"microseconds"`
> > }
> >
> > type Event interface {
> > GetName() string
> > GetTimestamp() Timestamp
> > }
> >
> > func UnmarshalEvent(data []byte) (Event, error) {
> > base := struct {
> > Name string `json:"event"`
> > EventTimestamp Timestamp `json:"timestamp"`
> > }{}
> > if err := json.Unmarshal(data, &base); err != nil {
> > return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
> > }
> >
> > switch base.Name {
> >
> > case "ACPI_DEVICE_OST":
> > event := struct {
> > Data AcpiDeviceOstEvent `json:"data"`
> > }{}
> >
> > if err := json.Unmarshal(data, &event); err != nil {
> > return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
> > }
> > event.Data.EventTimestamp = base.EventTimestamp
> > return &event.Data, nil
> >
> > // more cases here
> > }
> > return nil, errors.New("Failed to recognize event")
> > }
>
> While I understand the need to have some way to figure out exactly
> what type of event we're looking at before being able to unmarshal
> the JSON data, I don't like how we force users to go through a
> non-standard API for it.
>
> Here's how I think we should do it:
>
> 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, errors.New(fmt.Sprintf("Failed to decode event:
> %s", string(data)))
> }
>
> switch tmp.Name {
> case "ACPI_DEVICE_OST":
> return &AcpiDeviceOstEvent{}, nil
>
> // more cases here
> }
>
> return nil, errors.New("Failed to recognize event")
> }
>
> func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
> type eventData struct {
> Info ACPIOSTInfo `json:"info"`
> }
> type event struct {
> Name string `json:"event"`
> EventTimestamp Timestamp `json:"timestamp"`
> Data eventData `json:"data"`
> }
>
> tmp := event{}
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
> if tmp.Name != "ACPI_DEVICE_OST" {
> return errors.New("name")
> }
>
> s.EventTimestamp = tmp.EventTimestamp
> s.Info = tmp.Data.Info
> return nil
> }
>
> This way, client code can look like
>
> in := `{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
>
> e, err := qapi.GetEventType([]byte(in))
> if err != nil {
> panic(err)
> }
> // e is of type AcpiDeviceOstEvent
>
> err = json.Unmarshal([]byte(in), &e)
> if err != nil {
> panic(err)
> }
>
> where only the first part (figuring out the type of the event) needs
> custom APIs, and the remaining code looks just like your average JSON
> handling.
I don't think this kind of detail really needs to be exposed to
clients. Also parsing the same json doc twice just feels wrong.
I think using the dummy union structs is the right approach, but
I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
make it clear this is different from a normal 'UnmarshalJSON'
method.
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] 47+ messages in thread
* Re: [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go
2022-07-05 15:46 ` Andrea Bolognani
@ 2022-07-05 16:49 ` Daniel P. Berrangé
2022-08-17 15:00 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-05 16:49 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Tue, Jul 05, 2022 at 08:46:21AM -0700, Andrea Bolognani wrote:
> On Fri, Jun 17, 2022 at 02:19:31PM +0200, Victor Toso wrote:
> > +type EmptyCommandReturn struct {
> > + CommandId string `json:"id,omitempty"`
> > + Error *QapiError `json:"error,omitempty"`
> > + Name string `json:"-"`
> > +}
>
> Do we need a specific type for this? Can't we just generate an
> appropriately-named type for each of the commands that don't return
> any data? It's not like we would have to write that code manually :)
Yes, I think having an explicit named return struct even for commands
not /currently/ returning data is good, and anticipates future changes
that might add extra return data fields to the QAPI schema.
>
> > +func (r *EmptyCommandReturn) GetCommandName() string {
> > + return r.Name
> > +}
>
> Just like Event.GetName() and Command.GetName(), I'm not convinced we
> should have this.
>
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] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-05 16:35 ` Daniel P. Berrangé
@ 2022-07-06 9:28 ` Andrea Bolognani
2022-07-06 9:37 ` Daniel P. Berrangé
2022-08-17 16:06 ` Victor Toso
0 siblings, 2 replies; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-06 9:28 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > All this string manipulation looks sketchy. Is there some reason that
> > I'm not seeing preventing you for doing something like the untested
> > code below?
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > if s.HyperV != nil {
> > type union struct {
> > Discriminator string `json:"type"`
> > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > }
> > tmp := union {
> > Discriminator: "hyper-v",
> > HyperV: s.HyperV,
> > }
> > return json.Marshal(tmp)
> > } else if s.S390 != nil {
> > type union struct {
> > Discriminator string `json:"type"`
> > S390 GuestPanicInformationHyperV `json:"s390"`
> > }
> > tmp := union {
> > Discriminator: "s390",
> > S390: s.S390,
> > }
> > return json.Marshal(tmp)
> > }
> > return nil, errors.New("...")
> > }
>
> Using these dummy structs is the way I've approached the
> discriminated union issue in the libvirt Golang XML bindings
> and it works well. It is the bit I like the least, but it was
> the lesser of many evils, and on the plus side in the QEMU case
> it'll be auto-generated code.
It appears to be the standard way to approach the problem in Go. It
sort of comes naturally given how the APIs for marshal/unmarshal have
been defined.
> > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > type Alias GuestPanicInformation
> > > peek := struct {
> > > Alias
> > > Driver string `json:"type"`
> > > }{}
> > >
> > > if err := json.Unmarshal(data, &peek); err != nil {
> > > return err
> > > }
> > > *s = GuestPanicInformation(peek.Alias)
> > >
> > > switch peek.Driver {
> > >
> > > case "hyper-v":
> > > s.HyperV = new(GuestPanicInformationHyperV)
> > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > s.HyperV = nil
> > > return err
> > > }
> > > case "s390":
> > > s.S390 = new(GuestPanicInformationS390)
> > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > s.S390 = nil
> > > return err
> > > }
> > > }
> > > // Unrecognizer drivers are silently ignored.
> > > return nil
> >
> > This looks pretty reasonable, but since you're only using "peek" to
> > look at the discriminator you should be able to leave out the Alias
> > type entirely and perform the initial Unmarshal operation while
> > ignoring all other fields.
>
> Once you've defined the dummy structs for the Marshall case
> though, you might as well use them for Unmarshall too, so you're
> not parsing the JSON twice.
You're right, that is undesirable. What about something like this?
type GuestPanicInformation struct {
HyperV *GuestPanicInformationHyperV
S390 *GuestPanicInformationS390
}
type jsonGuestPanicInformation struct {
Discriminator string `json:"type"`
HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
S390 *GuestPanicInformationS390 `json:"s390"`
}
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
if (s.HyperV != nil && s.S390 != nil) ||
(s.HyperV == nil && s.S390 == nil) {
// client hasn't filled in the struct properly
return nil, errors.New("...")
}
tmp := jsonGuestPanicInformation{}
if s.HyperV != nil {
tmp.Discriminator = "hyper-v"
tmp.HyperV = s.HyperV
} else if s.S390 != nil {
tmp.Discriminator = "s390"
tmp.S390 = s.S390
}
return json.Marshal(tmp)
}
func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
tmp := jsonGuestPanicInformation{}
err := json.Unmarshal(data, &tmp)
if err != nil {
return err
}
switch tmp.Discriminator {
case "hyper-v":
if tmp.HyperV == nil {
return errors.New("...")
}
s.HyperV = tmp.HyperV
case "s390":
if tmp.S390 == nil {
return errors.New("...")
}
s.S390 = tmp.S390
}
// if we hit none of the cases above, that means the
// server has produced a variant we don't know about
return nil
}
This avoid parsing the JSON twice as well as having to define
multiple dummy structs, which keeps the code shorter and more
readable.
I've also thrown in some additional error checking for good measure,
ensuring that we abort when the input is completely nonsensical from
a semantical standpoint.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-06 9:28 ` Andrea Bolognani
@ 2022-07-06 9:37 ` Daniel P. Berrangé
2022-07-06 9:48 ` Daniel P. Berrangé
2022-08-17 16:06 ` Victor Toso
1 sibling, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-06 9:37 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > > All this string manipulation looks sketchy. Is there some reason that
> > > I'm not seeing preventing you for doing something like the untested
> > > code below?
> > >
> > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > if s.HyperV != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > > }
> > > tmp := union {
> > > Discriminator: "hyper-v",
> > > HyperV: s.HyperV,
> > > }
> > > return json.Marshal(tmp)
> > > } else if s.S390 != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > S390 GuestPanicInformationHyperV `json:"s390"`
> > > }
> > > tmp := union {
> > > Discriminator: "s390",
> > > S390: s.S390,
> > > }
> > > return json.Marshal(tmp)
> > > }
> > > return nil, errors.New("...")
> > > }
> >
> > Using these dummy structs is the way I've approached the
> > discriminated union issue in the libvirt Golang XML bindings
> > and it works well. It is the bit I like the least, but it was
> > the lesser of many evils, and on the plus side in the QEMU case
> > it'll be auto-generated code.
>
> It appears to be the standard way to approach the problem in Go. It
> sort of comes naturally given how the APIs for marshal/unmarshal have
> been defined.
>
> > > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > > type Alias GuestPanicInformation
> > > > peek := struct {
> > > > Alias
> > > > Driver string `json:"type"`
> > > > }{}
> > > >
> > > > if err := json.Unmarshal(data, &peek); err != nil {
> > > > return err
> > > > }
> > > > *s = GuestPanicInformation(peek.Alias)
> > > >
> > > > switch peek.Driver {
> > > >
> > > > case "hyper-v":
> > > > s.HyperV = new(GuestPanicInformationHyperV)
> > > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > > s.HyperV = nil
> > > > return err
> > > > }
> > > > case "s390":
> > > > s.S390 = new(GuestPanicInformationS390)
> > > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > > s.S390 = nil
> > > > return err
> > > > }
> > > > }
> > > > // Unrecognizer drivers are silently ignored.
> > > > return nil
> > >
> > > This looks pretty reasonable, but since you're only using "peek" to
> > > look at the discriminator you should be able to leave out the Alias
> > > type entirely and perform the initial Unmarshal operation while
> > > ignoring all other fields.
> >
> > Once you've defined the dummy structs for the Marshall case
> > though, you might as well use them for Unmarshall too, so you're
> > not parsing the JSON twice.
>
> You're right, that is undesirable. What about something like this?
>
> type GuestPanicInformation struct {
> HyperV *GuestPanicInformationHyperV
> S390 *GuestPanicInformationS390
> }
>
> type jsonGuestPanicInformation struct {
> Discriminator string `json:"type"`
> HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> S390 *GuestPanicInformationS390 `json:"s390"`
> }
It can possibly be even simpler with just embedding the real
struct
type jsonGuestPanicInformation struct {
Discriminator string
GuestPanicInformation
}
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> if (s.HyperV != nil && s.S390 != nil) ||
> (s.HyperV == nil && s.S390 == nil) {
> // client hasn't filled in the struct properly
> return nil, errors.New("...")
> }
>
> tmp := jsonGuestPanicInformation{}
>
> if s.HyperV != nil {
> tmp.Discriminator = "hyper-v"
> tmp.HyperV = s.HyperV
> } else if s.S390 != nil {
> tmp.Discriminator = "s390"
> tmp.S390 = s.S390
> }
>
> return json.Marshal(tmp)
> }
And...
var discriminator string
if s.HyperV != nil {
discriminator = "hyper-v"
} else if s.S390 != nil {
discriminator = "s390"
}
tmp := jsonGuestPanicInformation{ discriminator, s}
return json.Marshal(tmp)
>
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> tmp := jsonGuestPanicInformation{}
>
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
>
> switch tmp.Discriminator {
> case "hyper-v":
> if tmp.HyperV == nil {
> return errors.New("...")
> }
> s.HyperV = tmp.HyperV
> case "s390":
> if tmp.S390 == nil {
> return errors.New("...")
> }
> s.S390 = tmp.S390
> }
I'm not actually sure this works, because the first json.Unmarshal
call won't know which branch to try unmarhsalling. So it might be
unavoidable to parse twice. With the XML parser this wouldn't be
a problem as it has separated the parse phase and then fills the
struct after.
> // if we hit none of the cases above, that means the
> // server has produced a variant we don't know about
>
> return nil
> }
>
> This avoid parsing the JSON twice as well as having to define
> multiple dummy structs, which keeps the code shorter and more
> readable.
>
> I've also thrown in some additional error checking for good measure,
> ensuring that we abort when the input is completely nonsensical from
> a semantical standpoint.
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] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-06 9:37 ` Daniel P. Berrangé
@ 2022-07-06 9:48 ` Daniel P. Berrangé
2022-07-06 12:20 ` Andrea Bolognani
2022-08-17 16:25 ` Victor Toso
0 siblings, 2 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-06 9:48 UTC (permalink / raw)
To: Andrea Bolognani, Victor Toso, qemu-devel, Eric Blake,
Markus Armbruster, John Snow
On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > > > All this string manipulation looks sketchy. Is there some reason that
> > > > I'm not seeing preventing you for doing something like the untested
> > > > code below?
> > > >
> > > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > > if s.HyperV != nil {
> > > > type union struct {
> > > > Discriminator string `json:"type"`
> > > > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > > > }
> > > > tmp := union {
> > > > Discriminator: "hyper-v",
> > > > HyperV: s.HyperV,
> > > > }
> > > > return json.Marshal(tmp)
> > > > } else if s.S390 != nil {
> > > > type union struct {
> > > > Discriminator string `json:"type"`
> > > > S390 GuestPanicInformationHyperV `json:"s390"`
> > > > }
> > > > tmp := union {
> > > > Discriminator: "s390",
> > > > S390: s.S390,
> > > > }
> > > > return json.Marshal(tmp)
> > > > }
> > > > return nil, errors.New("...")
> > > > }
> > >
> > > Using these dummy structs is the way I've approached the
> > > discriminated union issue in the libvirt Golang XML bindings
> > > and it works well. It is the bit I like the least, but it was
> > > the lesser of many evils, and on the plus side in the QEMU case
> > > it'll be auto-generated code.
> >
> > It appears to be the standard way to approach the problem in Go. It
> > sort of comes naturally given how the APIs for marshal/unmarshal have
> > been defined.
> >
> > > > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > > > type Alias GuestPanicInformation
> > > > > peek := struct {
> > > > > Alias
> > > > > Driver string `json:"type"`
> > > > > }{}
> > > > >
> > > > > if err := json.Unmarshal(data, &peek); err != nil {
> > > > > return err
> > > > > }
> > > > > *s = GuestPanicInformation(peek.Alias)
> > > > >
> > > > > switch peek.Driver {
> > > > >
> > > > > case "hyper-v":
> > > > > s.HyperV = new(GuestPanicInformationHyperV)
> > > > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > > > s.HyperV = nil
> > > > > return err
> > > > > }
> > > > > case "s390":
> > > > > s.S390 = new(GuestPanicInformationS390)
> > > > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > > > s.S390 = nil
> > > > > return err
> > > > > }
> > > > > }
> > > > > // Unrecognizer drivers are silently ignored.
> > > > > return nil
> > > >
> > > > This looks pretty reasonable, but since you're only using "peek" to
> > > > look at the discriminator you should be able to leave out the Alias
> > > > type entirely and perform the initial Unmarshal operation while
> > > > ignoring all other fields.
> > >
> > > Once you've defined the dummy structs for the Marshall case
> > > though, you might as well use them for Unmarshall too, so you're
> > > not parsing the JSON twice.
> >
> > You're right, that is undesirable. What about something like this?
> >
> > type GuestPanicInformation struct {
> > HyperV *GuestPanicInformationHyperV
> > S390 *GuestPanicInformationS390
> > }
> >
> > type jsonGuestPanicInformation struct {
> > Discriminator string `json:"type"`
> > HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> > S390 *GuestPanicInformationS390 `json:"s390"`
> > }
>
> It can possibly be even simpler with just embedding the real
> struct
>
> type jsonGuestPanicInformation struct {
> Discriminator string
> GuestPanicInformation
> }
>
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > if (s.HyperV != nil && s.S390 != nil) ||
> > (s.HyperV == nil && s.S390 == nil) {
> > // client hasn't filled in the struct properly
> > return nil, errors.New("...")
> > }
> >
> > tmp := jsonGuestPanicInformation{}
> >
> > if s.HyperV != nil {
> > tmp.Discriminator = "hyper-v"
> > tmp.HyperV = s.HyperV
> > } else if s.S390 != nil {
> > tmp.Discriminator = "s390"
> > tmp.S390 = s.S390
> > }
> >
> > return json.Marshal(tmp)
> > }
>
> And...
>
> var discriminator string
> if s.HyperV != nil {
> discriminator = "hyper-v"
> } else if s.S390 != nil {
> discriminator = "s390"
> }
>
> tmp := jsonGuestPanicInformation{ discriminator, s}
> return json.Marshal(tmp)
>
> >
> > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > tmp := jsonGuestPanicInformation{}
> >
> > err := json.Unmarshal(data, &tmp)
> > if err != nil {
> > return err
> > }
> >
> > switch tmp.Discriminator {
> > case "hyper-v":
> > if tmp.HyperV == nil {
> > return errors.New("...")
> > }
> > s.HyperV = tmp.HyperV
> > case "s390":
> > if tmp.S390 == nil {
> > return errors.New("...")
> > }
> > s.S390 = tmp.S390
> > }
>
> I'm not actually sure this works, because the first json.Unmarshal
> call won't know which branch to try unmarhsalling. So it might be
> unavoidable to parse twice. With the XML parser this wouldn't be
> a problem as it has separated the parse phase and then fills the
> struct after.
Right afer sending, I remember how this is supposed to be done. It
involves use of 'json.RawMessage' eg examples at:
https://pkg.go.dev/encoding/json#example-RawMessage-Unmarshal
So it would look like:
type GuestPanicInformation struct {
HyperV *GuestPanicInformationHyperV
S390 *GuestPanicInformationS390
}
type jsonGuestPanicInformation struct {
Discriminator string `json:"type"`
Payload *json.RawMessage
}
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
var p *json.RawMesage
var err error
if s.HyperV != nil {
d = "hyper-v"
p, err = json.Marshal(s.HyperV)
} else if s.S390 != nil {
d = "s390"
p, err = json.Marshal(s.S390)
} else {
err = fmt.Errorf("No payload defined")
}
if err != nil {
return []byte{}, err
}
return json.Marshal(jsonGuestPanicInformation{d, p}), nil
}
func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
tmp := jsonGuestPanicInformation{}
err := json.Unmarshal(data, &tmp)
if err != nil {
return err
}
switch tmp.Discriminator {
case "hyper-v":
s.HyperV := GuestPanicInformationHyperV{}
err := json.Unmarshal(tmp.Payload, s.HyperV)
if err != nil {
return err
}
case "s390":
s.S390 := GuestPanicInformationS390{}
err := json.Unmarshal(tmp.Payload, s.S390)
if err != nil {
return err
}
}
return fmt.Errorf("Unknown type '%s'", tmp.Discriminator)
}
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] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-06 9:48 ` Daniel P. Berrangé
@ 2022-07-06 12:20 ` Andrea Bolognani
2022-08-17 16:25 ` Victor Toso
1 sibling, 0 replies; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-06 12:20 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Wed, Jul 06, 2022 at 10:48:06AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > tmp := jsonGuestPanicInformation{}
> > >
> > > err := json.Unmarshal(data, &tmp)
> > > if err != nil {
> > > return err
> > > }
> > >
> > > switch tmp.Discriminator {
> > > case "hyper-v":
> > > if tmp.HyperV == nil {
> > > return errors.New("...")
> > > }
> > > s.HyperV = tmp.HyperV
> > > case "s390":
> > > if tmp.S390 == nil {
> > > return errors.New("...")
> > > }
> > > s.S390 = tmp.S390
> > > }
> >
> > I'm not actually sure this works, because the first json.Unmarshal
> > call won't know which branch to try unmarhsalling. So it might be
> > unavoidable to parse twice. With the XML parser this wouldn't be
> > a problem as it has separated the parse phase and then fills the
> > struct after.
It does, because the struct it's filling with data
(jsonGuestPanicInformation) covers all possible cases. We then pick
only the part we care about and transfer it to the user-provided
return location.
> Right afer sending, I remember how this is supposed to be done. It
> involves use of 'json.RawMessage' eg examples at:
>
> https://pkg.go.dev/encoding/json#example-RawMessage-Unmarshal
>
> So it would look like:
>
> type GuestPanicInformation struct {
> HyperV *GuestPanicInformationHyperV
> S390 *GuestPanicInformationS390
> }
>
> type jsonGuestPanicInformation struct {
> Discriminator string `json:"type"`
> Payload *json.RawMessage
> }
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> var p *json.RawMesage
> var err error
> if s.HyperV != nil {
> d = "hyper-v"
> p, err = json.Marshal(s.HyperV)
> } else if s.S390 != nil {
> d = "s390"
> p, err = json.Marshal(s.S390)
> } else {
> err = fmt.Errorf("No payload defined")
> }
> if err != nil {
> return []byte{}, err
> }
>
> return json.Marshal(jsonGuestPanicInformation{d, p}), nil
> }
>
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> tmp := jsonGuestPanicInformation{}
>
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
>
> switch tmp.Discriminator {
> case "hyper-v":
> s.HyperV := GuestPanicInformationHyperV{}
> err := json.Unmarshal(tmp.Payload, s.HyperV)
> if err != nil {
> return err
> }
> case "s390":
> s.S390 := GuestPanicInformationS390{}
> err := json.Unmarshal(tmp.Payload, s.S390)
> if err != nil {
> return err
> }
> }
>
> return fmt.Errorf("Unknown type '%s'", tmp.Discriminator)
> }
I guess this version would work too, but I think it might still
perform more computation than the one I suggested?
json.RawMessage is a type alias for []byte, so I think the first call
to json.Unmarshal will have to go through the message to figure out
its structure and the contents of the discriminator field, then
tweak the original input so that only the part that hasn't been
consumed yet is returned. The second call to json.Marshal will then
parse that part, which means that the "inner" chunk of JSON ends up
being processed twice. In the approach I suggested, you go over the
entire JSON in one go.
Things might even out when you take into account allocating and
copying data around though. I'm not particularly interested in
splitting hair when it comes to the most efficient approach at this
point in time :) Knowing that we're not going through the entire JSON
twice is good enough.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-05 16:47 ` Daniel P. Berrangé
@ 2022-07-06 14:53 ` Andrea Bolognani
2022-07-06 15:07 ` Daniel P. Berrangé
2022-08-18 7:55 ` Victor Toso
1 sibling, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-06 14:53 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote:
> > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > > return s.EventTimestamp
> > > }
> >
> > Does this even need a getter? The struct member is public, and Go
> > code seems to favor direct member access.
>
> It satisfies the 'Event' interface, so you can fetch timestamp
> without needing to know the specific event struct you're dealing
> with.
Yeah but we're generating structs for all possible events ourselves
and we don't really expect external implementations of this
interface so I don't see what having this getter buys us over the
guarantee, that we have by construction, that all events will have a
public member with a specific name that contains the timestamp.
> I don't think this kind of detail really needs to be exposed to
> clients. Also parsing the same json doc twice just feels wrong.
>
> I think using the dummy union structs is the right approach, but
> I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> make it clear this is different from a normal 'UnmarshalJSON'
> method.
Fair point on avoiding parsing the JSON twice.
I still don't like the fact that we have to force clients to use a
non-standard API, or that the API for events has to be different from
that for unions. But Go won't let you add method implementations to
an interface, so we're kinda stuck.
The only alternative I can think of would be to define Event as
type Event struct {
Shutdown *ShutdownEvent
Reset *ResetEvent
// and so on
}
which wouldn't be too bad from client code, as you'd only have to
change from
e, err := EventFromJSON(in)
switch v := e.(type) {
case ShutdownEvent:
// do something with v
case ResetEvent:
// do something with v
// and so on
}
to
err := json.UnmarshalJSON(in, &e)
if e.Shutdown != nil {
// do something with e.Shutdown
} else if e.Reset != nil {
// do something with e.Reset
} // and so on
but that would mean each time we have to parse an event we'd end up
allocating room for ~50 pointers and only actually using one, which
is a massive waste.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-06 14:53 ` Andrea Bolognani
@ 2022-07-06 15:07 ` Daniel P. Berrangé
2022-07-06 16:22 ` Andrea Bolognani
0 siblings, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2022-07-06 15:07 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote:
> On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote:
> > > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > > > return s.EventTimestamp
> > > > }
> > >
> > > Does this even need a getter? The struct member is public, and Go
> > > code seems to favor direct member access.
> >
> > It satisfies the 'Event' interface, so you can fetch timestamp
> > without needing to know the specific event struct you're dealing
> > with.
>
> Yeah but we're generating structs for all possible events ourselves
> and we don't really expect external implementations of this
> interface so I don't see what having this getter buys us over the
> guarantee, that we have by construction, that all events will have a
> public member with a specific name that contains the timestamp.
Code doesn't neccessarily want to deal with the per-event type
structs. It is credible to want to work with the abstract 'Event'
interface in places and being able to get the Timestamp from that,
without figuring out what specific event struct to cast it to first.
> > I don't think this kind of detail really needs to be exposed to
> > clients. Also parsing the same json doc twice just feels wrong.
> >
> > I think using the dummy union structs is the right approach, but
> > I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> > make it clear this is different from a normal 'UnmarshalJSON'
> > method.
>
> Fair point on avoiding parsing the JSON twice.
>
> I still don't like the fact that we have to force clients to use a
> non-standard API, or that the API for events has to be different from
> that for unions. But Go won't let you add method implementations to
> an interface, so we're kinda stuck.
I think this specific case is out of scope for the "standard" JSON
APIs, and somewhere you'd expect APIs to do their own thing a layer
above, which is what we're doing here.
More importantly though what we're generating in terms of QAPI derived
APIs should be thought of as only the first step. Ultimately I would
not expect clients to be marshalling / unmarshalling these structs at
all. So from that POV I think the question of "non-standard" APIs is
largely irrelevant.
Instead we would be providing a "QMPClient" type with APIs for sending/
receiving instances of the 'Command'/'Response' interfaces, along with
callback(s) that receives instances of the 'Event' interface. Any JSON
marshalling is hidden behind the scenes as an internal imlp detail.
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] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-06 15:07 ` Daniel P. Berrangé
@ 2022-07-06 16:22 ` Andrea Bolognani
0 siblings, 0 replies; 47+ messages in thread
From: Andrea Bolognani @ 2022-07-06 16:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Victor Toso, qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Wed, Jul 06, 2022 at 04:07:43PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote:
> > Yeah but we're generating structs for all possible events ourselves
> > and we don't really expect external implementations of this
> > interface so I don't see what having this getter buys us over the
> > guarantee, that we have by construction, that all events will have a
> > public member with a specific name that contains the timestamp.
>
> Code doesn't neccessarily want to deal with the per-event type
> structs. It is credible to want to work with the abstract 'Event'
> interface in places and being able to get the Timestamp from that,
> without figuring out what specific event struct to cast it to first.
Makes sense.
> > I still don't like the fact that we have to force clients to use a
> > non-standard API, or that the API for events has to be different from
> > that for unions. But Go won't let you add method implementations to
> > an interface, so we're kinda stuck.
>
> I think this specific case is out of scope for the "standard" JSON
> APIs, and somewhere you'd expect APIs to do their own thing a layer
> above, which is what we're doing here.
>
> More importantly though what we're generating in terms of QAPI derived
> APIs should be thought of as only the first step. Ultimately I would
> not expect clients to be marshalling / unmarshalling these structs at
> all. So from that POV I think the question of "non-standard" APIs is
> largely irrelevant.
>
> Instead we would be providing a "QMPClient" type with APIs for sending/
> receiving instances of the 'Command'/'Response' interfaces, along with
> callback(s) that receives instances of the 'Event' interface. Any JSON
> marshalling is hidden behind the scenes as an internal imlp detail.
I will note that we want the marshalling/unmarshalling part and the
wire transfer part to be somewhat loosely coupled, to allow for use
cases that are not covered by the high-level client API, but overall
I now agree with you that for most users this shouldn't ultimately
matter :)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-07-05 15:45 ` Andrea Bolognani
@ 2022-08-17 14:04 ` Victor Toso
2022-08-19 16:27 ` Andrea Bolognani
0 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-08-17 14:04 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 7658 bytes --]
Hi,
On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> Sorry it took me a while to find the time to look into this!
(1.5 month later.. what can I say!) :)
> Overall this second iteration is a significant improvement over the
> initial one. There are still a few things that I think should be
> changed, so for the time being I'm going to comment mostly on the
> generated Go code and leave the details of the implementation for
> later.
Sure, and thanks.
> On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> > This patch handles QAPI alternate types and generates data structures
> > in Go that handles it.
> >
> > At this moment, there are 5 alternates in qemu/qapi, they are:
> > * BlockDirtyBitmapMergeSource
> > * Qcow2OverlapChecks
> > * BlockdevRef
> > * BlockdevRefOrNull
> > * StrOrNull
>
> I personally don't think it's very useful to list all the alternate
> types in the commit message, or even mention how many there are. You
> do this for all other types too, and it seems to me that it's just an
> opportunity for incosistent information to make their way to the git
> repository - what if a new type is introduced between the time your
> series is queued and merged? I'd say just drop this part.
No issue on my side in dropping this bits.
> > 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"
>
> Let me just say that I absolutely *love* how you've added these bits
> comparing the QAPI / Go representations as well as usage. They really
> help a lot understanding what the generator is trying to achieve :)
Thanks!
> > +// Creates a decoder that errors on unknown Fields
> > +// Returns true if successfully decoded @from string @into type
> > +// Returns false without error is failed with "unknown field"
> > +// Returns false with error is a different error was found
> > +func StrictDecode(into interface{}, from []byte) error {
> > + dec := json.NewDecoder(strings.NewReader(string(from)))
> > + dec.DisallowUnknownFields()
> > +
> > + if err := dec.Decode(into); err != nil {
> > + return err
> > + }
> > + return nil
> > +}
>
> The documentation doesn't seem to be consistent with how the
> function actually works: AFAICT it returns false *with an
> error* for any failure, including those caused by unknown
> fields being present in the input JSON.
You are correct, documentation of this helper function needs to
be fixed if we keep the helper function, as you made a good point
about backwards-compatible decoding a struct that might have
introduced extra fields in a newer version.
> Looking at the generated code:
>
> > type BlockdevRef struct {
> > Definition *BlockdevOptions
> > Reference *string
> > }
> >
> > func (s BlockdevRef) MarshalJSON() ([]byte, error) {
> > if s.Definition != nil {
> > return json.Marshal(s.Definition)
> > } else if s.Reference != nil {
> > return json.Marshal(s.Reference)
> > }
> > return nil, errors.New("BlockdevRef has empty fields")
>
> Returning an error if no field is set is good. Can we be more
> strict and returning one if more than one is set as well? That
> feels better than just picking the first one.
Sure.
> > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> > // Check for json-null first
> > if string(data) == "null" {
> > return errors.New(`null not supported for BlockdevRef`)
> > }
> > // Check for BlockdevOptions
> > {
> > s.Definition = new(BlockdevOptions)
> > if err := StrictDecode(s.Definition, data); err == nil {
> > return nil
> > }
>
> The use of StrictDecode() here means that we won't be able to
> parse an alternate produced by a version of QEMU where
> BlockdevOptions has gained additional fields, doesn't it?
That's correct. This means that with this RFCv2 proposal, qapi-go
based on qemu version 7.1 might not be able to decode a qmp
message from qemu version 7.2 if it has introduced a new field.
This needs fixing, not sure yet the way to go.
> Considering that we will happily parse such a BlockdevOptions
> outside of the context of BlockdevRef, I think we should be
> consistent and allow the same to happen here.
StrictDecode is only used with alternates because, unlike unions,
Alternate types don't have a 'discriminator' field that would
allow us to know what data type to expect.
With this in mind, theoretically speaking, we could have very
similar struct types as Alternate fields and we have to find on
runtime which type is that underlying byte stream.
So, to reply to your suggestion, if we allow BlockdevRef without
StrictDecode we might find ourselves in a situation that it
matched a few fields of BlockdevOptions but it the byte stream
was actually another type.
So, I acknowledge that current version of this patch is not
correct but we still need to look into the fields and see if
there is a perfect match with the information that we have.
> > s.Definition = nil
> > }
> > // Check for string
> > {
> > s.Reference = new(string)
> > if err := StrictDecode(s.Reference, data); err == nil {
> > return nil
> > }
> > s.Reference = nil
> > }
> >
> > return errors.New(fmt.Sprintf("Can't convert to BlockdevRef: %s", string(data)))
>
> On a similar note to the MarshalJSON comment above, I'm not
> sure this is right.
>
> If we find that more than one field of the alternate is set, we
> should error out - that's just invalid JSON we're dealing with.
With StrictDecode (or something similar) there shouldn't be
multiple fields being set on the Go structure. Once it finds a
match, it returns nil (no error)
> But if we couldn't find any, that might be valid JSON that's
> been produced by a version of QEMU that introduced additional
> options to the alternate. In the spirit of "be liberal in what
> you accept", I think we should probably not reject that? Of
> course then the client code will have to look like
>
> if r.Definition != nil {
> // do something with r.Definition
> } else if r.Reference != nil {
> // do something with r.Reference
> } else {
> // we don't recognize this - error out
> }
>
> but the same is going to be true for enums, events and everything
> else that can be extended in a backwards-compatible fashion.
The client code looking like above is not much different than
if err := json.Unmarshal([]byte(input), &r); err != nil {
// error out: bad json? incompatibility issue?
} else if r.Definition != nil {
// do something with r.Definition
} else if r.Reference != nil {
// do something with r.Reference
} else {
// empty might be valid though, e.g: JSON null
}
My main concern with alternate logic is not putting data that
should go to r.Reference into r.Definition and vice versa.
I took note of all your suggestions, I'll be reworking this. I'll
revisit the problem with StrictDecode together with addressing my
reply to Daniel:
https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg03140.html
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-07-05 15:46 ` Andrea Bolognani
@ 2022-08-17 14:24 ` Victor Toso
2022-08-29 11:53 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-08-17 14:24 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]
Hi,
On Tue, Jul 05, 2022 at 08:46:34AM -0700, Andrea Bolognani wrote:
> I've commented in detail to the single patches, just a couple of
> additional points.
>
> On Fri, Jun 17, 2022 at 02:19:24PM +0200, Victor Toso wrote:
> > * 7) Flat structs by removing embed types. Discussion with Andrea
> > Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
> >
> > No one required it but I decided to give it a try. Major issue that
> > I see with this approach is to have generated a few 'Base' structs
> > that are now useless. Overall, less nested structs seems better to
> > me. Opnions?
> >
> > Example:
> > | /* This is now useless, should be removed? */
> > | type InetSocketAddressBase struct {
> > | Host string `json:"host"`
> > | Port string `json:"port"`
> > | }
>
> Can we somehow keep track, in the generator, of types that are
> only used as building blocks for other types, and prevent them
> from showing up in the generated code?
I'm not 100% sure it is good to remove them from generated code
because technically it is a valid qapi type. If all @base types
are embed types and they don't show in other way or form, sure we
can remove them from generated code... I'm not sure if it is
possible to guarantee this.
But yes, if possible, I'd like to remove what seems useless type
definitions.
> Finally, looking at the repository containing the generated
> code I see that the generated type are sorted by kind, e.g. all
> unions are in a file, all events in another one and so on. I
> believe the structure should match more closely that of the
> QAPI schema, so e.g. block-related types should all go in one
> file, net-related types in another one and so on.
That's something I don't mind adding but some hardcoded mapping
is needed. If you look into git history of qapi/ folder, .json
files can come and go, types be moved around, etc. So, we need to
proper map types in a way that the generated code would be kept
stable even if qapi files would have been rearranged. What I
proposed was only the simplest solution.
Also, the generator takes a qapi-schema.json as input. We are
more focused in qemu/qapi/qapi-schema.json generated coded but
would not hurt to think we could even use it for qemu-guest-agent
from qemu/qga/qapi-schema.json -- this to say that the hardcoded
mapping needs to take into account non qemu qapi schemas too.
> Looking forward to the next iteration :)
Me too, thanks again!
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go
2022-07-05 16:49 ` Daniel P. Berrangé
@ 2022-08-17 15:00 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-17 15:00 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Andrea Bolognani, qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
Hi,
On Tue, Jul 05, 2022 at 05:49:22PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:46:21AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:31PM +0200, Victor Toso wrote:
> > > +type EmptyCommandReturn struct {
> > > + CommandId string `json:"id,omitempty"`
> > > + Error *QapiError `json:"error,omitempty"`
> > > + Name string `json:"-"`
> > > +}
> >
> > Do we need a specific type for this? Can't we just generate an
> > appropriately-named type for each of the commands that don't return
> > any data? It's not like we would have to write that code manually :)
>
> Yes, I think having an explicit named return struct even for commands
> not /currently/ returning data is good, and anticipates future changes
> that might add extra return data fields to the QAPI schema.
Sure.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-06 9:28 ` Andrea Bolognani
2022-07-06 9:37 ` Daniel P. Berrangé
@ 2022-08-17 16:06 ` Victor Toso
1 sibling, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-17 16:06 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Daniel P. Berrangé,
qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1: Type: text/plain, Size: 6329 bytes --]
Hi,
On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > > All this string manipulation looks sketchy. Is there some reason that
> > > I'm not seeing preventing you for doing something like the untested
> > > code below?
> > >
> > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > if s.HyperV != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > > }
> > > tmp := union {
> > > Discriminator: "hyper-v",
> > > HyperV: s.HyperV,
> > > }
> > > return json.Marshal(tmp)
> > > } else if s.S390 != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > S390 GuestPanicInformationHyperV `json:"s390"`
> > > }
> > > tmp := union {
> > > Discriminator: "s390",
> > > S390: s.S390,
> > > }
> > > return json.Marshal(tmp)
> > > }
> > > return nil, errors.New("...")
> > > }
> >
> > Using these dummy structs is the way I've approached the
> > discriminated union issue in the libvirt Golang XML bindings
> > and it works well. It is the bit I like the least, but it was
> > the lesser of many evils, and on the plus side in the QEMU case
> > it'll be auto-generated code.
>
> It appears to be the standard way to approach the problem in Go. It
> sort of comes naturally given how the APIs for marshal/unmarshal have
> been defined.
Yep, string manipulation was bad choice. Some sort of anonymous
struct is a better fit. So I'll be changing this ...
> > > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > > type Alias GuestPanicInformation
> > > > peek := struct {
> > > > Alias
> > > > Driver string `json:"type"`
> > > > }{}
> > > >
> > > > if err := json.Unmarshal(data, &peek); err != nil {
> > > > return err
> > > > }
> > > > *s = GuestPanicInformation(peek.Alias)
> > > >
> > > > switch peek.Driver {
> > > >
> > > > case "hyper-v":
> > > > s.HyperV = new(GuestPanicInformationHyperV)
> > > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > > s.HyperV = nil
> > > > return err
> > > > }
> > > > case "s390":
> > > > s.S390 = new(GuestPanicInformationS390)
> > > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > > s.S390 = nil
> > > > return err
> > > > }
> > > > }
> > > > // Unrecognizer drivers are silently ignored.
> > > > return nil
> > >
> > > This looks pretty reasonable, but since you're only using "peek" to
> > > look at the discriminator you should be able to leave out the Alias
> > > type entirely and perform the initial Unmarshal operation while
> > > ignoring all other fields.
> >
> > Once you've defined the dummy structs for the Marshall case
> > though, you might as well use them for Unmarshall too, so you're
> > not parsing the JSON twice.
>
> You're right, that is undesirable. What about something like this?
>
> type GuestPanicInformation struct {
> HyperV *GuestPanicInformationHyperV
> S390 *GuestPanicInformationS390
> }
>
> type jsonGuestPanicInformation struct {
> Discriminator string `json:"type"`
> HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> S390 *GuestPanicInformationS390 `json:"s390"`
> }
I didn't test this so I could be wrong but, I think this should
not work in case you want to remove the string manipulation.
The marshalling of either HyperV or S390 fields would return a
JSON Object but QAPI spec expects the fields at the same level as
the discriminator's type [0]. So, with this you still need some
string manipulation to remove the extra {}, like I did poorly
without any comment 0:-)
[0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L358
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> if (s.HyperV != nil && s.S390 != nil) ||
> (s.HyperV == nil && s.S390 == nil) {
> // client hasn't filled in the struct properly
> return nil, errors.New("...")
> }
>
> tmp := jsonGuestPanicInformation{}
>
> if s.HyperV != nil {
> tmp.Discriminator = "hyper-v"
> tmp.HyperV = s.HyperV
> } else if s.S390 != nil {
> tmp.Discriminator = "s390"
> tmp.S390 = s.S390
> }
>
> return json.Marshal(tmp)
> }
>
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> tmp := jsonGuestPanicInformation{}
>
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
>
> switch tmp.Discriminator {
> case "hyper-v":
> if tmp.HyperV == nil {
> return errors.New("...")
> }
> s.HyperV = tmp.HyperV
> case "s390":
> if tmp.S390 == nil {
> return errors.New("...")
> }
> s.S390 = tmp.S390
> }
> // if we hit none of the cases above, that means the
> // server has produced a variant we don't know about
>
> return nil
> }
>
> This avoid parsing the JSON twice as well as having to define
> multiple dummy structs, which keeps the code shorter and more
> readable.
If not too verbose, I'd still use anonymous structs whenever
possible. They are created for a given scope, they are basically
self documented in that given context and don't polluted
package's namespace.
> I've also thrown in some additional error checking for good
> measure, ensuring that we abort when the input is completely
> nonsensical from a semantical standpoint.
Thanks!
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-07-06 9:48 ` Daniel P. Berrangé
2022-07-06 12:20 ` Andrea Bolognani
@ 2022-08-17 16:25 ` Victor Toso
2022-08-19 7:20 ` Victor Toso
1 sibling, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-08-17 16:25 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Andrea Bolognani, qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1: Type: text/plain, Size: 5638 bytes --]
On Wed, Jul 06, 2022 at 10:48:06AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > You're right, that is undesirable. What about something like this?
> > >
> > > type GuestPanicInformation struct {
> > > HyperV *GuestPanicInformationHyperV
> > > S390 *GuestPanicInformationS390
> > > }
> > >
> > > type jsonGuestPanicInformation struct {
> > > Discriminator string `json:"type"`
> > > HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> > > S390 *GuestPanicInformationS390 `json:"s390"`
> > > }
> >
> > It can possibly be even simpler with just embedding the real
> > struct
> >
> > type jsonGuestPanicInformation struct {
> > Discriminator string
> > GuestPanicInformation
> > }
Similar to what I said in previous email (same thread) to Andrea,
this would not work because the end result does not match with
QAPI spec, where HyperV or S390 fields should be at the same
level as 'type'.
If we embed either HyperV or S390, then it should work, like:
tmp := struct {
GuestPanicInformationHyperV
Discriminator string "type"
}{}
But I intend to try the json.RawMessage too as with description
it seems like we can avoid looking the whole json data twice.
> > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > if (s.HyperV != nil && s.S390 != nil) ||
> > > (s.HyperV == nil && s.S390 == nil) {
> > > // client hasn't filled in the struct properly
> > > return nil, errors.New("...")
> > > }
> > >
> > > tmp := jsonGuestPanicInformation{}
> > >
> > > if s.HyperV != nil {
> > > tmp.Discriminator = "hyper-v"
> > > tmp.HyperV = s.HyperV
> > > } else if s.S390 != nil {
> > > tmp.Discriminator = "s390"
> > > tmp.S390 = s.S390
> > > }
> > >
> > > return json.Marshal(tmp)
> > > }
> >
> > And...
> >
> > var discriminator string
> > if s.HyperV != nil {
> > discriminator = "hyper-v"
> > } else if s.S390 != nil {
> > discriminator = "s390"
> > }
> >
> > tmp := jsonGuestPanicInformation{ discriminator, s}
> > return json.Marshal(tmp)
> >
> > >
> > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > tmp := jsonGuestPanicInformation{}
> > >
> > > err := json.Unmarshal(data, &tmp)
> > > if err != nil {
> > > return err
> > > }
> > >
> > > switch tmp.Discriminator {
> > > case "hyper-v":
> > > if tmp.HyperV == nil {
> > > return errors.New("...")
> > > }
> > > s.HyperV = tmp.HyperV
> > > case "s390":
> > > if tmp.S390 == nil {
> > > return errors.New("...")
> > > }
> > > s.S390 = tmp.S390
> > > }
> >
> > I'm not actually sure this works, because the first json.Unmarshal
> > call won't know which branch to try unmarhsalling. So it might be
> > unavoidable to parse twice. With the XML parser this wouldn't be
> > a problem as it has separated the parse phase and then fills the
> > struct after.
>
> Right afer sending, I remember how this is supposed to be done. It
> involves use of 'json.RawMessage' eg examples at:
>
> https://pkg.go.dev/encoding/json#example-RawMessage-Unmarshal
>
> So it would look like:
>
> type GuestPanicInformation struct {
> HyperV *GuestPanicInformationHyperV
> S390 *GuestPanicInformationS390
> }
>
> type jsonGuestPanicInformation struct {
> Discriminator string `json:"type"`
> Payload *json.RawMessage
> }
>
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> var p *json.RawMesage
> var err error
> if s.HyperV != nil {
> d = "hyper-v"
> p, err = json.Marshal(s.HyperV)
> } else if s.S390 != nil {
> d = "s390"
> p, err = json.Marshal(s.S390)
> } else {
> err = fmt.Errorf("No payload defined")
> }
> if err != nil {
> return []byte{}, err
> }
>
> return json.Marshal(jsonGuestPanicInformation{d, p}), nil
> }
>
>
>
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> tmp := jsonGuestPanicInformation{}
>
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
>
> switch tmp.Discriminator {
> case "hyper-v":
> s.HyperV := GuestPanicInformationHyperV{}
> err := json.Unmarshal(tmp.Payload, s.HyperV)
> if err != nil {
> return err
> }
> case "s390":
> s.S390 := GuestPanicInformationS390{}
> err := json.Unmarshal(tmp.Payload, s.S390)
> if err != nil {
> return err
> }
> }
>
> return fmt.Errorf("Unknown type '%s'", tmp.Discriminator)
> }
>
> 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 :|
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-05 15:45 ` Andrea Bolognani
2022-07-05 16:47 ` Daniel P. Berrangé
@ 2022-08-18 7:47 ` Victor Toso
1 sibling, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-18 7:47 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 8853 bytes --]
Hi,
On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> On Fri, Jun 17, 2022 at 02:19:29PM +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.
> >
> > At the moment of this writing, this patch generates 51 structures (50
> > events)
> >
> > Example:
> >
> > qapi:
> > | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> > | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
> >
> > go:
> > | type MemoryDeviceSizeChangeEvent struct {
> > | EventTimestamp 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]"
> > | }
>
> Generated code:
>
> > type AcpiDeviceOstEvent struct {
> > EventTimestamp Timestamp `json:"-"`
>
> Any reason for naming this EventTimestamp as opposed to just
> Timestamp?
Yes, perhaps one that can be workaround in the next iteration.
IIRC, I've added the type prefix to avoid the possibility of name
clash with generated fields (like Info below).
> > Info ACPIOSTInfo `json:"info"`
> > }
This happened with Command's Id that clashed with an Id for one
or several other commands so I've changed it to "CommandId" and
thought it would be wise to do the same for non generated fields.
> > func (s *AcpiDeviceOstEvent) GetName() string {
> > return "ACPI_DEVICE_OST"
> > }
>
> Getters in Go don't usually start with "Get", so this could be just
> Name(). And pointer receivers are usually only recommended when the
> underlying object needs to be modified, which is not the case here.
Yeah, thanks. I'll change it.
> > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > return s.EventTimestamp
> > }
>
> Does this even need a getter? The struct member is public, and Go
> code seems to favor direct member access.
As Daniel pointed out, just for the sake of working with a Event
interface.
> > type Timestamp struct {
> > Seconds int64 `json:"seconds"`
> > Microseconds int64 `json:"microseconds"`
> > }
> >
> > type Event interface {
> > GetName() string
> > GetTimestamp() Timestamp
> > }
> >
> > func MarshalEvent(e Event) ([]byte, error) {
> > baseStruct := struct {
> > Name string `json:"event"`
> > EventTimestamp Timestamp `json:"timestamp"`
> > }{
> > Name: e.GetName(),
> > EventTimestamp: e.GetTimestamp(),
> > }
> > base, err := json.Marshal(baseStruct)
> > if err != nil {
> > return []byte{}, err
> > }
> >
> > dataStruct := struct {
> > Payload Event `json:"data"`
> > }{
> > Payload: e,
> > }
> > data, err := json.Marshal(dataStruct)
> > if err != nil {
> > return []byte{}, err
> > }
> >
> > if len(data) == len(`{"data":{}}`) {
> > return base, nil
> > }
> >
> > // Combines Event's base and data in a single JSON object
> > result := fmt.Sprintf("%s,%s", base[:len(base)-1], data[1:])
> > return []byte(result), nil
> > }
>
> I have the same concerns about the string manipulation going on
> here as I had for unions. Here's an alternative implementation,
> and this time I've even tested it :)
Yup. I agree that string manipulation was bad decision. I'll
change it here too.
> func (s AcpiDeviceOstEvent) MarshalJSON() ([]byte, error) {
> type eventData struct {
> Info ACPIOSTInfo `json:"info"`
> }
> type event struct {
> Name string `json:"event"`
> Timestamp Timestamp `json:"timestamp"`
> Data eventData `json:"data"`
> }
>
> tmp := event{
> Name: "ACPI_DEVICE_OST",
> Timestamp: s.EventTimestamp,
> Data: eventData{
> Info: s.Info,
> },
> }
> return json.Marshal(tmp)
> }
>
> > func UnmarshalEvent(data []byte) (Event, error) {
> > base := struct {
> > Name string `json:"event"`
> > EventTimestamp Timestamp `json:"timestamp"`
> > }{}
> > if err := json.Unmarshal(data, &base); err != nil {
> > return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
> > }
> >
> > switch base.Name {
> >
> > case "ACPI_DEVICE_OST":
> > event := struct {
> > Data AcpiDeviceOstEvent `json:"data"`
> > }{}
> >
> > if err := json.Unmarshal(data, &event); err != nil {
> > return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
> > }
> > event.Data.EventTimestamp = base.EventTimestamp
> > return &event.Data, nil
> >
> > // more cases here
> > }
> > return nil, errors.New("Failed to recognize event")
> > }
>
> While I understand the need to have some way to figure out exactly
> what type of event we're looking at before being able to unmarshal
> the JSON data, I don't like how we force users to go through a
> non-standard API for it.
>
> Here's how I think we should do it:
>
> 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, errors.New(fmt.Sprintf("Failed to decode event:
> %s", string(data)))
> }
>
> switch tmp.Name {
> case "ACPI_DEVICE_OST":
> return &AcpiDeviceOstEvent{}, nil
>
> // more cases here
> }
>
> return nil, errors.New("Failed to recognize event")
> }
>
> func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
> type eventData struct {
> Info ACPIOSTInfo `json:"info"`
> }
> type event struct {
> Name string `json:"event"`
> EventTimestamp Timestamp `json:"timestamp"`
> Data eventData `json:"data"`
> }
>
> tmp := event{}
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
> if tmp.Name != "ACPI_DEVICE_OST" {
> return errors.New("name")
> }
>
> s.EventTimestamp = tmp.EventTimestamp
> s.Info = tmp.Data.Info
> return nil
> }
>
> This way, client code can look like
>
> in := `{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
>
> e, err := qapi.GetEventType([]byte(in))
> if err != nil {
> panic(err)
> }
> // e is of type AcpiDeviceOstEvent
>
> err = json.Unmarshal([]byte(in), &e)
> if err != nil {
> panic(err)
> }
>
> where only the first part (figuring out the type of the event) needs
> custom APIs, and the remaining code looks just like your average JSON
> handling.
I'll reply to this bit in the other email of this thread.
> Speaking of client code, in the commit message you have
>
> > 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 the use of string comparison for
> the purpose of going from a generic Event instance to a specific one:
> a better way would be to use Go's type switch feature, such as
>
> switch m := e.(type) {
> case *MemoryDeviceSizeChangeEvent:
> // m.QomPath == "/machine/unattached/device[2]"
> }
Yes, this seems much better/align with Go code. Many thanks for
this!
> In fact, I don't really see a reason to provide the Name() getter
> outside of possibly diagnostics, and given the potential of it being
> misused I would argue that it might be better not to have it.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
2022-07-05 16:47 ` Daniel P. Berrangé
2022-07-06 14:53 ` Andrea Bolognani
@ 2022-08-18 7:55 ` Victor Toso
1 sibling, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-18 7:55 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Andrea Bolognani, qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1: Type: text/plain, Size: 6511 bytes --]
Hi,
On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:29PM +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.
> > >
> > > At the moment of this writing, this patch generates 51 structures (50
> > > events)
> > >
> > > Example:
> > >
> > > qapi:
> > > | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> > > | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
> > >
> > > go:
> > > | type MemoryDeviceSizeChangeEvent struct {
> > > | EventTimestamp 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]"
> > > | }
>
>
> > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > > return s.EventTimestamp
> > > }
> >
> > Does this even need a getter? The struct member is public, and Go
> > code seems to favor direct member access.
>
> It satisfies the 'Event' interface, so you can fetch timestamp
> without needing to know the specific event struct you're dealing
> with.
>
> >
> > > type Timestamp struct {
> > > Seconds int64 `json:"seconds"`
> > > Microseconds int64 `json:"microseconds"`
> > > }
> > >
> > > type Event interface {
> > > GetName() string
> > > GetTimestamp() Timestamp
> > > }
> > >
>
>
> > > func UnmarshalEvent(data []byte) (Event, error) {
> > > base := struct {
> > > Name string `json:"event"`
> > > EventTimestamp Timestamp `json:"timestamp"`
> > > }{}
> > > if err := json.Unmarshal(data, &base); err != nil {
> > > return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
> > > }
> > >
> > > switch base.Name {
> > >
> > > case "ACPI_DEVICE_OST":
> > > event := struct {
> > > Data AcpiDeviceOstEvent `json:"data"`
> > > }{}
> > >
> > > if err := json.Unmarshal(data, &event); err != nil {
> > > return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
> > > }
> > > event.Data.EventTimestamp = base.EventTimestamp
> > > return &event.Data, nil
> > >
> > > // more cases here
> > > }
> > > return nil, errors.New("Failed to recognize event")
> > > }
> >
> > While I understand the need to have some way to figure out exactly
> > what type of event we're looking at before being able to unmarshal
> > the JSON data, I don't like how we force users to go through a
> > non-standard API for it.
> >
> > Here's how I think we should do it:
> >
> > 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, errors.New(fmt.Sprintf("Failed to decode event:
> > %s", string(data)))
> > }
> >
> > switch tmp.Name {
> > case "ACPI_DEVICE_OST":
> > return &AcpiDeviceOstEvent{}, nil
> >
> > // more cases here
> > }
> >
> > return nil, errors.New("Failed to recognize event")
> > }
> >
> > func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
> > type eventData struct {
> > Info ACPIOSTInfo `json:"info"`
> > }
> > type event struct {
> > Name string `json:"event"`
> > EventTimestamp Timestamp `json:"timestamp"`
> > Data eventData `json:"data"`
> > }
> >
> > tmp := event{}
> > err := json.Unmarshal(data, &tmp)
> > if err != nil {
> > return err
> > }
> > if tmp.Name != "ACPI_DEVICE_OST" {
> > return errors.New("name")
> > }
> >
> > s.EventTimestamp = tmp.EventTimestamp
> > s.Info = tmp.Data.Info
> > return nil
> > }
> >
> > This way, client code can look like
> >
> > in := `{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
> >
> > e, err := qapi.GetEventType([]byte(in))
> > if err != nil {
> > panic(err)
> > }
> > // e is of type AcpiDeviceOstEvent
> >
> > err = json.Unmarshal([]byte(in), &e)
> > if err != nil {
> > panic(err)
> > }
> >
> > where only the first part (figuring out the type of the event) needs
> > custom APIs, and the remaining code looks just like your average JSON
> > handling.
>
> I don't think this kind of detail really needs to be exposed to
> clients. Also parsing the same json doc twice just feels wrong.
>
> I think using the dummy union structs is the right approach, but
> I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> make it clear this is different from a normal 'UnmarshalJSON'
> method.
The current method signatures are:
func MarshalEvent(e Event) ([]byte, error)
func UnmarshalEvent(data []byte) (Event, error)
the suggestion is to change it to
func EventToJSON(e Event) ([]byte, error)
func EventFromJSON(data []byte) (Event, error)
or
func JSONFromEvent(e Event) ([]byte, error)
func EventFromJSON(data []byte) (Event, error)
? :)
I'm not picky with names so, what you find is best is okay for
me.
I'll be changing the function to avoid string manipulation in
favor of some anonymous structs.
Thanks you all for the review, really appreciate it!
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-06-27 15:29 ` Markus Armbruster
@ 2022-08-18 8:04 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-18 8:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, John Snow, Andrea Bolognani,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 4360 bytes --]
Hi,
On Mon, Jun 27, 2022 at 05:29:26PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi Markus,
> >
> > On Mon, Jun 27, 2022 at 09:15:53AM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > Hi,
> >> >
> >> > This is the second iteration of RFC v1:
> >> > https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html
> >> >
> >> >
> >> > # What this is about?
> >> >
> >> > To generate a simple Golang interface that could communicate with QEMU
> >> > over QMP. The Go code that is generated is meant to be used as the bare
> >> > bones to exchange QMP messages.
> >> >
> >> > The goal is to have this as a Go module in QEMU gitlab namespace,
> >> > similar to what have been done to pyhon-qemu-qmp
> >> > https://gitlab.com/qemu-project/python-qemu-qmp
> >>
> >> Aspects of review:
> >>
> >> (1) Impact on common code, if any
> >>
> >> I care, because any messes made there are likely to affect me down
> >> the road.
> >
> > For the first version of the Go generated interface, my goal is
> > to have something that works and can be considered alpha to other
> > Go projects.
> >
> > With this first version, I don't want to bring huge changes to
> > the python library or to the QAPI spec and its usage in QEMU.
> > Unless someone finds that a necessity.
> >
> > So I hope (1) is simple :)
> >
> >> (2) The generated Go code
> >>
> >> Is it (close to) what we want long term? If not, is it good enough
> >> short term, and how could we make necessary improvements?
> >>
> >> I'd prefer to leave this to folks who actually know their Go.
> >> (3) General Python sanity
> >>
> >> We need eyes, but not necessarily mine. Any takers?
> >>
> >> [...]
> >>
> >> > scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++
> >> > scripts/qapi/main.py | 2 +
> >> > 2 files changed, 767 insertions(+)
> >> > create mode 100644 scripts/qapi/golang.py
> >>
> >> This adds a new generator and calls it from generate(), i.e.
> >> review aspect (1) is empty. "Empty" is a quick & easy way to
> >> get my ACK!
> >>
> >> No tests?
> >
> > I've added tests but on the qapi-go module, those are the files
> > with _test.go prefix on them. Example for commands:
> >
> > https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/commands_test.go
> >
> > Should the generator itself have tests or offloading that to the
> > qapi-go seems reasonable?
>
> Offloading may be reasonable, but how am I to run the tests then?
> Documentation should tell me.
>
> We have roughly three kinds of tests so far:
>
> 1. Front end tests in tests/qapi-schema
>
> 2. Unit tests in tests/unit/
>
> To find them:
>
> $ git-grep '#include ".*qapi-.*\.h"' tests/unit/
>
> 3. Many tests talking QMP in tests/qtest/
I'm thinking on the tests in QEMU side. Perhaps adding something
with Avocado that generates the qapi-go and communicates with a
running QEMU with that generated Go module?
One thing that I try to keep in mind is to not add Go
dependencies in QEMU and this Golang work is not internal to QEMU
itself.
> Since you leave the front end alone, you don't need the first
> kind.
>
> The other two kinds are less clear.
I'm open for suggestions. I thought that, if we have a qapi-go Go
module in Gitlab's qemu-project namespace, we could leverage most
of the tests to the consumer of the actual generated code but I
agree that it is necessary to have something in qemu too.
> >> No documentation?
> >
> > Yes, this iteration removed the documentation of the generated
> > types. I'm a bit sad about that. I want to consume the
> > documentation in the QAPI files to provide the latest info from
> > types/fields but we can't 'copy' it, the only solution is 'refer'
> > to it with hyperlink, which I haven't done yet.
>
> Two kinds of documentation: generated documentation for the generated Go
> code, and documentation about the generator. I was thinking of the
> latter. Specifically, docs/devel/qapi-code-gen.rst section "Code
> generation". Opinions on its usefulness differ. I like it.
Me too. I'll add documentation for the next iteration, thanks for
pointing it out.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-08-17 16:25 ` Victor Toso
@ 2022-08-19 7:20 ` Victor Toso
2022-08-19 15:25 ` Andrea Bolognani
0 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-08-19 7:20 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Andrea Bolognani, qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1.1: Type: text/plain, Size: 2214 bytes --]
Hi,
On Wed, Aug 17, 2022 at 06:25:56PM +0200, Victor Toso wrote:
> On Wed, Jul 06, 2022 at 10:48:06AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > > You're right, that is undesirable. What about something like this?
> > > >
> > > > type GuestPanicInformation struct {
> > > > HyperV *GuestPanicInformationHyperV
> > > > S390 *GuestPanicInformationS390
> > > > }
> > > >
> > > > type jsonGuestPanicInformation struct {
> > > > Discriminator string `json:"type"`
> > > > HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> > > > S390 *GuestPanicInformationS390 `json:"s390"`
> > > > }
> > >
> > > It can possibly be even simpler with just embedding the real
> > > struct
> > >
> > > type jsonGuestPanicInformation struct {
> > > Discriminator string
> > > GuestPanicInformation
> > > }
>
> Similar to what I said in previous email (same thread) to Andrea,
> this would not work because the end result does not match with
> QAPI spec, where HyperV or S390 fields should be at the same
> level as 'type'.
>
> If we embed either HyperV or S390, then it should work, like:
>
> tmp := struct {
> GuestPanicInformationHyperV
> Discriminator string "type"
> }{}
>
> But I intend to try the json.RawMessage too as with description
> it seems like we can avoid looking the whole json data twice.
For the same reason, I could not use json.RawMessage, sadly.
As Andrea pointed out before, json.RawMessage is just an alias
for []byte. We need a field for that (so, it can't be embed) and
the contents of the JSON need to match that field's name.
I've removed the string manipulation and used a few anonymous
structs instead. I'm attaching a main.go program that tests this
behavior together with input checks that Andrea suggested. This
is more or less how the generated code will look like in the next
iteration but in case one want's to find a nicer/shorter
solution, I'm all ears :)
Cheers,
Victor
[-- Attachment #1.2: main.go --]
[-- Type: text/plain, Size: 3182 bytes --]
package main
import (
"encoding/json"
"errors"
"fmt"
"strings"
)
type QCryptoBlockOptionsQCow struct {
KeySecret *string `json:"key-secret,omitempty"`
}
type QCryptoBlockOptionsLUKS struct {
KeySecret *string `json:"key-secret,omitempty"`
}
type QCryptoBlockOpenOptions struct {
// Variants fields
Qcow *QCryptoBlockOptionsQCow `json:"-"`
Luks *QCryptoBlockOptionsLUKS `json:"-"`
}
func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
var bytes []byte
var err error
if s.Qcow != nil {
tmp := struct {
QCryptoBlockOptionsQCow
Discriminator string `json:"format"`
}{
QCryptoBlockOptionsQCow: *s.Qcow,
Discriminator: "qcow",
}
if bytes, err = json.Marshal(tmp); err != nil {
return nil, err
}
}
if s.Luks != nil {
if len(bytes) != 0 {
return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
}
tmp := struct {
QCryptoBlockOptionsLUKS
Discriminator string `json:"format"`
}{
QCryptoBlockOptionsLUKS: *s.Luks,
Discriminator: "luks",
}
if bytes, err = json.Marshal(tmp); err != nil {
return nil, err
}
}
if len(bytes) == 0 {
return nil, errors.New(`null not supported for QCryptoBlockOpenOptions`)
}
return bytes, nil
}
func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
tmp := struct {
Discriminator string `json:"format"`
}{}
if err := json.Unmarshal(data, &tmp); err != nil {
return err
}
switch tmp.Discriminator {
case "qcow":
s.Qcow = &QCryptoBlockOptionsQCow{}
if err := json.Unmarshal(data, s.Qcow); err != nil {
s.Qcow = nil
return err
}
case "luks":
s.Luks = &QCryptoBlockOptionsLUKS{}
if err := json.Unmarshal(data, s.Luks); err != nil {
s.Luks = nil
return err
}
}
return nil
}
func main() {
jsonLuks := `{"key-secret":"my luks secret is here","format":"luks"}`
jsonQcow := `{"key-secret":"my qcow secret is here","format":"qcow"}`
r := QCryptoBlockOpenOptions{}
if err := json.Unmarshal([]byte(jsonLuks), &r); err != nil {
panic(err)
} else if r.Luks == nil || r.Qcow != nil {
panic(fmt.Sprintf("Wrong: %v", r))
} else if b, err := json.Marshal(&r); err != nil {
panic(err)
} else if string(b) != jsonLuks {
panic(string(b))
}
r = QCryptoBlockOpenOptions{}
if err := json.Unmarshal([]byte(jsonQcow), &r); err != nil {
panic(err)
} else if r.Luks != nil || r.Qcow == nil {
panic(fmt.Sprintf("Wrong: %v", r))
} else if b, err := json.Marshal(&r); err != nil {
panic(err)
} else if string(b) != jsonQcow {
panic(string(b))
}
r = QCryptoBlockOpenOptions{}
if _, err := json.Marshal(&r); err == nil {
panic("No fields set should be an error")
} else if !strings.Contains(err.Error(), "null not supported") {
panic(err)
}
qcowSecret := "my-qcow-secret-is-here"
luksSecret := "my-luks-secret-is-here"
r = QCryptoBlockOpenOptions{
Qcow: &QCryptoBlockOptionsQCow{
KeySecret: &qcowSecret,
},
Luks: &QCryptoBlockOptionsLUKS{
KeySecret: &luksSecret,
},
}
if _, err := json.Marshal(&r); err == nil {
panic("multiple fields set should be an error")
} else if !strings.Contains(err.Error(), "multiple fields set for") {
panic(err)
}
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-08-19 7:20 ` Victor Toso
@ 2022-08-19 15:25 ` Andrea Bolognani
2022-08-22 6:33 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-08-19 15:25 UTC (permalink / raw)
To: Victor Toso
Cc: Daniel P. Berrangé,
qemu-devel, Eric Blake, Markus Armbruster, John Snow
On Fri, Aug 19, 2022 at 09:20:52AM +0200, Victor Toso wrote:
> > > On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > > > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > > > You're right, that is undesirable. What about something like this?
> > > > >
> > > > > type GuestPanicInformation struct {
> > > > > HyperV *GuestPanicInformationHyperV
> > > > > S390 *GuestPanicInformationS390
> > > > > }
> > > > >
> > > > > type jsonGuestPanicInformation struct {
> > > > > Discriminator string `json:"type"`
> > > > > HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> > > > > S390 *GuestPanicInformationS390 `json:"s390"`
> > > > > }
> > > >
> > > > It can possibly be even simpler with just embedding the real
> > > > struct
> > > >
> > > > type jsonGuestPanicInformation struct {
> > > > Discriminator string
> > > > GuestPanicInformation
> > > > }
> >
> > Similar to what I said in previous email (same thread) to Andrea,
> > this would not work because the end result does not match with
> > QAPI spec, where HyperV or S390 fields should be at the same
> > level as 'type'.
Yeah, you're absolutely correct, I was misreading the schema and
suggested an implementation that couldn't possibly work.
> > If we embed either HyperV or S390, then it should work, like:
> >
> > tmp := struct {
> > GuestPanicInformationHyperV
> > Discriminator string "type"
> > }{}
>
> For the same reason, I could not use json.RawMessage, sadly.
>
> As Andrea pointed out before, json.RawMessage is just an alias
> for []byte. We need a field for that (so, it can't be embed) and
> the contents of the JSON need to match that field's name.
Right. It would work if unions looked like
{ "type": "...", "data": { ... }}
with the "data" part having different fields based on the value of
"type", but not for the flat JSON dictionaries that are used for QAPI
unions.
> func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
> var bytes []byte
> var err error
> if s.Qcow != nil {
> tmp := struct {
> QCryptoBlockOptionsQCow
> Discriminator string `json:"format"`
> }{
> QCryptoBlockOptionsQCow: *s.Qcow,
> Discriminator: "qcow",
> }
> if bytes, err = json.Marshal(tmp); err != nil {
> return nil, err
> }
> }
> if s.Luks != nil {
> if len(bytes) != 0 {
> return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
> }
Why are you checking this here? I would just have a check like
if s.Qcow != nil && s.Luks != nil {
return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
}
at the top of the function, so that you can abort early and cheaply
if the user has provided invalid input instead of having to wait
after one of the fields has already been serialized.
> tmp := struct {
> QCryptoBlockOptionsLUKS
> Discriminator string `json:"format"`
> }{
> QCryptoBlockOptionsLUKS: *s.Luks,
> Discriminator: "luks",
> }
> if bytes, err = json.Marshal(tmp); err != nil {
> return nil, err
> }
> }
> if len(bytes) == 0 {
> return nil, errors.New(`null not supported for QCryptoBlockOpenOptions`)
> }
Similarly, this should be checked early. So the complete check could
be turned into
if (s.Qcow != nil && s.Luks != nil) || (s.Qcow == nil && s.Luks == nil) {
return nil, errors.New("must set exactly one field")
}
You should have enough information to produce such a check when
generating the function, right? If making sure all possible
combinations are covered up ends up being too complicated, something
like
var setFields int
if s.Field1 != nil {
setFields += 1
}
if s.Field2 != nil {
setFields += 1
}
// ...
if setFields != 1 {
return nil, errors.New("must set exactly one field")
}
would also work.
> func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
> tmp := struct {
> Discriminator string `json:"format"`
> }{}
> if err := json.Unmarshal(data, &tmp); err != nil {
> return err
> }
> switch tmp.Discriminator {
> case "qcow":
> s.Qcow = &QCryptoBlockOptionsQCow{}
> if err := json.Unmarshal(data, s.Qcow); err != nil {
> s.Qcow = nil
> return err
> }
> case "luks":
> s.Luks = &QCryptoBlockOptionsLUKS{}
> if err := json.Unmarshal(data, s.Luks); err != nil {
> s.Luks = nil
> return err
> }
> }
> return nil
> }
Alternatively, you could define a struct that covers all possible
fields and deserialize everything in one go, then copy the various
parts over to the appropriate struct:
func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
tmp := struct {
Discriminator string `json:"format"`
KeySecret *string `json:"key-secret,omitempty"`
}{}
if err := json.Unmarshal(data, &tmp); err != nil {
return err
}
switch tmp.Discriminator {
case "qcow":
s.Qcow = &QCryptoBlockOptionsQCow{}
s.Qcow.KeySecret = tmp.KeySecret
case "luks":
s.Luks = &QCryptoBlockOptionsLUKS{}
s.Luks.KeySecret = tmp.KeySecret
}
return nil
}
Honestly I'm unclear on which option is better. Parsing the JSON
twice, as you're doing in your version, could be problematic when the
document is large; on the other hand, my approach will probably
result in more copies of the same data being created.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-08-17 14:04 ` Victor Toso
@ 2022-08-19 16:27 ` Andrea Bolognani
2022-08-22 6:59 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Andrea Bolognani @ 2022-08-19 16:27 UTC (permalink / raw)
To: Victor Toso
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
> On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> > > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> > > // Check for json-null first
> > > if string(data) == "null" {
> > > return errors.New(`null not supported for BlockdevRef`)
> > > }
> > > // Check for BlockdevOptions
> > > {
> > > s.Definition = new(BlockdevOptions)
> > > if err := StrictDecode(s.Definition, data); err == nil {
> > > return nil
> > > }
> >
> > The use of StrictDecode() here means that we won't be able to
> > parse an alternate produced by a version of QEMU where
> > BlockdevOptions has gained additional fields, doesn't it?
>
> That's correct. This means that with this RFCv2 proposal, qapi-go
> based on qemu version 7.1 might not be able to decode a qmp
> message from qemu version 7.2 if it has introduced a new field.
>
> This needs fixing, not sure yet the way to go.
>
> > Considering that we will happily parse such a BlockdevOptions
> > outside of the context of BlockdevRef, I think we should be
> > consistent and allow the same to happen here.
>
> StrictDecode is only used with alternates because, unlike unions,
> Alternate types don't have a 'discriminator' field that would
> allow us to know what data type to expect.
>
> With this in mind, theoretically speaking, we could have very
> similar struct types as Alternate fields and we have to find on
> runtime which type is that underlying byte stream.
>
> So, to reply to your suggestion, if we allow BlockdevRef without
> StrictDecode we might find ourselves in a situation that it
> matched a few fields of BlockdevOptions but it the byte stream
> was actually another type.
IIUC your concern is that the QAPI schema could gain a new type,
TotallyNotBlockdevOptions, which looks exactly like BlockdevOptions
except for one or more extra fields.
If QEMU then produced a JSON like
{ "description": { /* a TotallyNotBlockdevOptions here */ } }
and we'd try to deserialize it with Go code like
ref := BlockdevRef{}
json.Unmarsal(&ref)
we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
valid BlockdevOptions, dropping the extra fields in the process.
Does that correctly describe the reason why you feel that the use of
StrictDecode is necessary?
If so, I respectfully disagree :)
If the client code is expecting a BlockdevRef as the return value of
a command and QEMU is producing something that is *not* a BlockdevRef
instead, that's an obvious bug in QEMU. If the client code is
expecting a BlockdevRef as the return value of a command that is
specified *not* to return a BlockdevRef, that's an obvious bug in the
client code.
In neither case it should be the responsibility of the SDK to
second-guess the declared intent, especially when it's perfectly
valid for a type to be extended in a backwards-compatible way by
adding fields to it.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
2022-08-19 15:25 ` Andrea Bolognani
@ 2022-08-22 6:33 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-22 6:33 UTC (permalink / raw)
To: Andrea Bolognani
Cc: Daniel P. Berrangé,
qemu-devel, Eric Blake, Markus Armbruster, John Snow
[-- Attachment #1: Type: text/plain, Size: 5350 bytes --]
Hi,
On Fri, Aug 19, 2022 at 10:25:26AM -0500, Andrea Bolognani wrote:
> > func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
> > var bytes []byte
> > var err error
> > if s.Qcow != nil {
> > tmp := struct {
> > QCryptoBlockOptionsQCow
> > Discriminator string `json:"format"`
> > }{
> > QCryptoBlockOptionsQCow: *s.Qcow,
> > Discriminator: "qcow",
> > }
> > if bytes, err = json.Marshal(tmp); err != nil {
> > return nil, err
> > }
> > }
> > if s.Luks != nil {
> > if len(bytes) != 0 {
> > return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
> > }
>
> Why are you checking this here? I would just have a check like
>
> if s.Qcow != nil && s.Luks != nil {
> return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
> }
>
> at the top of the function, so that you can abort early and
> cheaply if the user has provided invalid input instead of
> having to wait after one of the fields has already been
> serialized.
In general, I too prefer to return early! So I agree with you
that it is nicer. At the same time, I was thinking a bit from the
code generator point of view and the overall expected diffs when
generating new code. More below.
> > tmp := struct {
> > QCryptoBlockOptionsLUKS
> > Discriminator string `json:"format"`
> > }{
> > QCryptoBlockOptionsLUKS: *s.Luks,
> > Discriminator: "luks",
> > }
> > if bytes, err = json.Marshal(tmp); err != nil {
> > return nil, err
> > }
> > }
> > if len(bytes) == 0 {
> > return nil, errors.New(`null not supported for QCryptoBlockOpenOptions`)
> > }
>
> Similarly, this should be checked early. So the complete check
> could be turned into
>
> if (s.Qcow != nil && s.Luks != nil) || (s.Qcow == nil && s.Luks == nil) {
> return nil, errors.New("must set exactly one field")
> }
Main problem with this approach is that there is some big unions
like BlockdevOptions, this means that we would have to repeat all
fields by the number fields times (40+ in BlockdevOptions' case).
> You should have enough information to produce such a check when
> generating the function, right?
We do!
> If making sure all possible combinations are covered up ends up
> being too complicated, something
> like
>
> var setFields int
> if s.Field1 != nil {
> setFields += 1
> }
> if s.Field2 != nil {
> setFields += 1
> }
> // ...
> if setFields != 1 {
> return nil, errors.New("must set exactly one field")
> }
>
> would also work.
I started with this approach actually but then I realized that we
can just keep the if checks and instead of counter, do check the
variable bytes []byte. So, do you think that the counter is
actually preferred instead of inner check? I don't mind changing
it.
> > func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
> > tmp := struct {
> > Discriminator string `json:"format"`
> > }{}
> > if err := json.Unmarshal(data, &tmp); err != nil {
> > return err
> > }
> > switch tmp.Discriminator {
> > case "qcow":
> > s.Qcow = &QCryptoBlockOptionsQCow{}
> > if err := json.Unmarshal(data, s.Qcow); err != nil {
> > s.Qcow = nil
> > return err
> > }
> > case "luks":
> > s.Luks = &QCryptoBlockOptionsLUKS{}
> > if err := json.Unmarshal(data, s.Luks); err != nil {
> > s.Luks = nil
> > return err
> > }
> > }
> > return nil
> > }
>
> Alternatively, you could define a struct that covers all
> possible fields and deserialize everything in one go, then copy
> the various parts over to the appropriate struct:
>
> func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
> tmp := struct {
> Discriminator string `json:"format"`
> KeySecret *string `json:"key-secret,omitempty"`
> }{}
> if err := json.Unmarshal(data, &tmp); err != nil {
> return err
> }
> switch tmp.Discriminator {
> case "qcow":
> s.Qcow = &QCryptoBlockOptionsQCow{}
> s.Qcow.KeySecret = tmp.KeySecret
> case "luks":
> s.Luks = &QCryptoBlockOptionsLUKS{}
> s.Luks.KeySecret = tmp.KeySecret
I think this one adds a bit more complexity and I'm not convinced
that the gains are worth.
For example, with types that differ over fields with the same
name? We would need to move that inside the anonymous struct
somehow;
For example,if Luks's KeySecret was KeyScretType we would have to
use KeySecretLuks *KeySecretType. Still, both of them would
likely be inside of "key-secret" json object (so, same key for
two different fields, not really sure how that would work out)
> }
> return nil
> }
>
> Honestly I'm unclear on which option is better. Parsing the
> JSON twice, as you're doing in your version, could be
> problematic when the document is large; on the other hand, my
> approach will probably result in more copies of the same data
> being created.
It does sound nice to parse it once but if we really want to do
that, we would need to work with Golang's JSON decoder [0]. IMHO,
parsing twice with in-memory data might be okay for the moment
while we are trying to keep things simple and later we could
benchmark against a custom decoder [0] in the future.
[0] https://pkg.go.dev/encoding/json#Decoder
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-08-19 16:27 ` Andrea Bolognani
@ 2022-08-22 6:59 ` Victor Toso
2022-08-29 11:27 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Victor Toso @ 2022-08-22 6:59 UTC (permalink / raw)
To: Andrea Bolognani
Cc: qemu-devel, Eric Blake, Markus Armbruster, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 5414 bytes --]
Hi,
On Fri, Aug 19, 2022 at 11:27:13AM -0500, Andrea Bolognani wrote:
> On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
> > On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> > > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> > > > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> > > > // Check for json-null first
> > > > if string(data) == "null" {
> > > > return errors.New(`null not supported for BlockdevRef`)
> > > > }
> > > > // Check for BlockdevOptions
> > > > {
> > > > s.Definition = new(BlockdevOptions)
> > > > if err := StrictDecode(s.Definition, data); err == nil {
> > > > return nil
> > > > }
> > >
> > > The use of StrictDecode() here means that we won't be able to
> > > parse an alternate produced by a version of QEMU where
> > > BlockdevOptions has gained additional fields, doesn't it?
> >
> > That's correct. This means that with this RFCv2 proposal, qapi-go
> > based on qemu version 7.1 might not be able to decode a qmp
> > message from qemu version 7.2 if it has introduced a new field.
> >
> > This needs fixing, not sure yet the way to go.
> >
> > > Considering that we will happily parse such a BlockdevOptions
> > > outside of the context of BlockdevRef, I think we should be
> > > consistent and allow the same to happen here.
> >
> > StrictDecode is only used with alternates because, unlike unions,
> > Alternate types don't have a 'discriminator' field that would
> > allow us to know what data type to expect.
> >
> > With this in mind, theoretically speaking, we could have very
> > similar struct types as Alternate fields and we have to find on
> > runtime which type is that underlying byte stream.
> >
> > So, to reply to your suggestion, if we allow BlockdevRef without
> > StrictDecode we might find ourselves in a situation that it
> > matched a few fields of BlockdevOptions but it the byte stream
> > was actually another type.
>
> IIUC your concern is that the QAPI schema could gain a new
> type, TotallyNotBlockdevOptions, which looks exactly like
> BlockdevOptions except for one or more extra fields.
>
> If QEMU then produced a JSON like
>
> { "description": { /* a TotallyNotBlockdevOptions here */ } }
>
> and we'd try to deserialize it with Go code like
>
> ref := BlockdevRef{}
> json.Unmarsal(&ref)
>
> we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
> valid BlockdevOptions, dropping the extra fields in the process.
>
> Does that correctly describe the reason why you feel that the use of
> StrictDecode is necessary?
Not quite. The problem here is related to the Alternate types of
the QAPI specification [0], just to name a simple in-use example,
BlockdevRefOrNul [1].
[0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L387
[1] https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/block-core.json#L4349
To exemplify the problem that I try to solve with StrictDecode,
let's say there is a DeviceRef alternate type that looks like:
{ 'alternate': 'DeviceRef',
'data': { 'memory': 'BlockdevRefInMemory',
'disk': 'BlockdevRefInDisk',
'cloud': 'BlockdevRefInCloud' } }
Just a quick recap, at runtime we don't have data's payload name
(e.g: disk). We need to check the actual data and try to find
what is the payload type.
type BlockdevRefInMemory struct {
Name *string
Size uint64
Start uint64
End uint64
}
type BlockdevRefInDisk struct {
Name *string
Size uint64
Path *string
}
type BlockdevRefInCloud struct {
Name *string
Size uint64
Uri *string
}
All types have unique fields but they all share some fields too.
Golang, without StrictDecode would happily decode a "disk"
payload into either "memory" or "cloud" fields, matching only
what the json provides and ignoring the rest. StrictDecode would
error if the payload had fields that don't belong to that Type so
we could try to find a perfect match.
While this should work reliably with current version of QEMU's
qapi-schema.json, it is not future error proof... It is just a
bit better than not checking at all.
The overall expectations is that, if the fields have too much in
common, it is likely they should have been tagged as 'union'
instead of 'alternate'. Yet, because alternate types have this
flexibility, we need to be extra careful.
I'm still thinking about this in a way that would not give too
much hassle when considering a generated code that talks with
older/newer qemu where some fields might have been added/removed.
> If so, I respectfully disagree :)
>
> If the client code is expecting a BlockdevRef as the return
> value of a command and QEMU is producing something that is
> *not* a BlockdevRef instead, that's an obvious bug in QEMU. If
> the client code is expecting a BlockdevRef as the return value
> of a command that is specified *not* to return a BlockdevRef,
> that's an obvious bug in the client code.
>
> In neither case it should be the responsibility of the SDK to
> second-guess the declared intent, especially when it's
> perfectly valid for a type to be extended in a
> backwards-compatible way by adding fields to it.
Yes, the SDK should consider valid QMP messages. This specific
case is just a bit more complex qapi type that SDK needs to
worry.
Thanks for your input!
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-08-22 6:59 ` Victor Toso
@ 2022-08-29 11:27 ` Markus Armbruster
2022-08-29 13:31 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2022-08-29 11:27 UTC (permalink / raw)
To: Victor Toso
Cc: Andrea Bolognani, qemu-devel, Eric Blake, Markus Armbruster,
John Snow, Daniel P . Berrangé
Victor Toso <victortoso@redhat.com> writes:
> Hi,
>
> On Fri, Aug 19, 2022 at 11:27:13AM -0500, Andrea Bolognani wrote:
>> On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
>> > On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
>> > > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
>> > > > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
>> > > > // Check for json-null first
>> > > > if string(data) == "null" {
>> > > > return errors.New(`null not supported for BlockdevRef`)
>> > > > }
>> > > > // Check for BlockdevOptions
>> > > > {
>> > > > s.Definition = new(BlockdevOptions)
>> > > > if err := StrictDecode(s.Definition, data); err == nil {
>> > > > return nil
>> > > > }
>> > >
>> > > The use of StrictDecode() here means that we won't be able to
>> > > parse an alternate produced by a version of QEMU where
>> > > BlockdevOptions has gained additional fields, doesn't it?
>> >
>> > That's correct. This means that with this RFCv2 proposal, qapi-go
>> > based on qemu version 7.1 might not be able to decode a qmp
>> > message from qemu version 7.2 if it has introduced a new field.
>> >
>> > This needs fixing, not sure yet the way to go.
>> >
>> > > Considering that we will happily parse such a BlockdevOptions
>> > > outside of the context of BlockdevRef, I think we should be
>> > > consistent and allow the same to happen here.
>> >
>> > StrictDecode is only used with alternates because, unlike unions,
>> > Alternate types don't have a 'discriminator' field that would
>> > allow us to know what data type to expect.
>> >
>> > With this in mind, theoretically speaking, we could have very
>> > similar struct types as Alternate fields and we have to find on
>> > runtime which type is that underlying byte stream.
>> >
>> > So, to reply to your suggestion, if we allow BlockdevRef without
>> > StrictDecode we might find ourselves in a situation that it
>> > matched a few fields of BlockdevOptions but it the byte stream
>> > was actually another type.
>>
>> IIUC your concern is that the QAPI schema could gain a new
>> type, TotallyNotBlockdevOptions, which looks exactly like
>> BlockdevOptions except for one or more extra fields.
>>
>> If QEMU then produced a JSON like
>>
>> { "description": { /* a TotallyNotBlockdevOptions here */ } }
>>
>> and we'd try to deserialize it with Go code like
>>
>> ref := BlockdevRef{}
>> json.Unmarsal(&ref)
>>
>> we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
>> valid BlockdevOptions, dropping the extra fields in the process.
>>
>> Does that correctly describe the reason why you feel that the use of
>> StrictDecode is necessary?
>
> Not quite. The problem here is related to the Alternate types of
> the QAPI specification [0], just to name a simple in-use example,
> BlockdevRefOrNul [1].
>
> [0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L387
> [1] https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/block-core.json#L4349
>
> To exemplify the problem that I try to solve with StrictDecode,
> let's say there is a DeviceRef alternate type that looks like:
>
> { 'alternate': 'DeviceRef',
> 'data': { 'memory': 'BlockdevRefInMemory',
> 'disk': 'BlockdevRefInDisk',
> 'cloud': 'BlockdevRefInCloud' } }
>
> Just a quick recap, at runtime we don't have data's payload name
> (e.g: disk). We need to check the actual data and try to find
> what is the payload type.
>
> type BlockdevRefInMemory struct {
> Name *string
> Size uint64
> Start uint64
> End uint64
> }
>
> type BlockdevRefInDisk struct {
> Name *string
> Size uint64
> Path *string
> }
>
> type BlockdevRefInCloud struct {
> Name *string
> Size uint64
> Uri *string
> }
>
> All types have unique fields but they all share some fields too.
Quick intercession (I merely skimmed the review thread; forgive me if
it's not useful or not new):
An alternate type is like a union type, except there is no
discriminator on the wire. Instead, the branch to use is inferred
from the value. An alternate can only express a choice between types
represented differently on the wire.
This is docs/devel/qapi-code-gen.rst. Implied there: the inference is
based on the JSON type *only*, i.e. no two branches can have the same
JSON type on the wire. Since all complex types (struct or union) are
JSON object on the wire, at most one alternate branch can be of complex
type.
More sophisticated inference would be possible if we need it. So far we
haven't.
> Golang, without StrictDecode would happily decode a "disk"
> payload into either "memory" or "cloud" fields, matching only
> what the json provides and ignoring the rest. StrictDecode would
> error if the payload had fields that don't belong to that Type so
> we could try to find a perfect match.
>
> While this should work reliably with current version of QEMU's
> qapi-schema.json, it is not future error proof... It is just a
> bit better than not checking at all.
>
> The overall expectations is that, if the fields have too much in
> common, it is likely they should have been tagged as 'union'
> instead of 'alternate'. Yet, because alternate types have this
> flexibility, we need to be extra careful.
>
> I'm still thinking about this in a way that would not give too
> much hassle when considering a generated code that talks with
> older/newer qemu where some fields might have been added/removed.
>
>> If so, I respectfully disagree :)
>>
>> If the client code is expecting a BlockdevRef as the return
>> value of a command and QEMU is producing something that is
>> *not* a BlockdevRef instead, that's an obvious bug in QEMU. If
>> the client code is expecting a BlockdevRef as the return value
>> of a command that is specified *not* to return a BlockdevRef,
>> that's an obvious bug in the client code.
>>
>> In neither case it should be the responsibility of the SDK to
>> second-guess the declared intent, especially when it's
>> perfectly valid for a type to be extended in a
>> backwards-compatible way by adding fields to it.
>
> Yes, the SDK should consider valid QMP messages. This specific
> case is just a bit more complex qapi type that SDK needs to
> worry.
>
> Thanks for your input!
> Victor
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-08-17 14:24 ` Victor Toso
@ 2022-08-29 11:53 ` Markus Armbruster
2022-08-29 14:05 ` Victor Toso
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2022-08-29 11:53 UTC (permalink / raw)
To: Victor Toso
Cc: Andrea Bolognani, qemu-devel, Eric Blake, John Snow,
Daniel P . Berrangé
Victor Toso <victortoso@redhat.com> writes:
> Hi,
>
> On Tue, Jul 05, 2022 at 08:46:34AM -0700, Andrea Bolognani wrote:
>> I've commented in detail to the single patches, just a couple of
>> additional points.
>>
>> On Fri, Jun 17, 2022 at 02:19:24PM +0200, Victor Toso wrote:
>> > * 7) Flat structs by removing embed types. Discussion with Andrea
>> > Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
>> >
>> > No one required it but I decided to give it a try. Major issue that
>> > I see with this approach is to have generated a few 'Base' structs
>> > that are now useless. Overall, less nested structs seems better to
>> > me. Opnions?
>> >
>> > Example:
>> > | /* This is now useless, should be removed? */
>> > | type InetSocketAddressBase struct {
>> > | Host string `json:"host"`
>> > | Port string `json:"port"`
>> > | }
>>
>> Can we somehow keep track, in the generator, of types that are
>> only used as building blocks for other types, and prevent them
>> from showing up in the generated code?
>
> I'm not 100% sure it is good to remove them from generated code
> because technically it is a valid qapi type. If all @base types
> are embed types and they don't show in other way or form, sure we
> can remove them from generated code... I'm not sure if it is
> possible to guarantee this.
>
> But yes, if possible, I'd like to remove what seems useless type
> definitions.
The existing C generators have to generate all the types, because the
generated code is for QEMU's own use, where we need all the types.
The existing introspection generator generates only the types visible in
QAPI/QMP introspection.
The former generate for internal use (where we want all the types), and
the latter for external use (where only the types visible in the
external interface are actually useful).
>> Finally, looking at the repository containing the generated
>> code I see that the generated type are sorted by kind, e.g. all
>> unions are in a file, all events in another one and so on. I
>> believe the structure should match more closely that of the
>> QAPI schema, so e.g. block-related types should all go in one
>> file, net-related types in another one and so on.
>
> That's something I don't mind adding but some hardcoded mapping
> is needed. If you look into git history of qapi/ folder, .json
> files can come and go, types be moved around, etc. So, we need to
> proper map types in a way that the generated code would be kept
> stable even if qapi files would have been rearranged. What I
> proposed was only the simplest solution.
>
> Also, the generator takes a qapi-schema.json as input. We are
> more focused in qemu/qapi/qapi-schema.json generated coded but
> would not hurt to think we could even use it for qemu-guest-agent
> from qemu/qga/qapi-schema.json -- this to say that the hardcoded
> mapping needs to take into account non qemu qapi schemas too.
In the beginning, the QAPI schema was monolithic. qga/qapi-schema.json
still is.
When keeping everything in a single qapi-schema.json became unwieldy, we
split it into "modules" tied together with a simple include directive.
Generated code remained monolithic.
When monolithic generated code became too annoying (touch schema,
recompile everything), we made it match the module structure: code for
FOO.json goes into *-FOO.c and *-FOO.h, where the *-FOO.h #include the
generated headers for the .json modules FOO.json includes.
Schema code motion hasn't been much of a problem. Moving from FOO.json
to one of the modules it includes is transparent. Non-transparent moves
are relatively rare as long as the split into modules actually makes
sense.
>> Looking forward to the next iteration :)
>
> Me too, thanks again!
>
> Cheers,
> Victor
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-08-29 11:27 ` Markus Armbruster
@ 2022-08-29 13:31 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-29 13:31 UTC (permalink / raw)
To: Markus Armbruster
Cc: Andrea Bolognani, qemu-devel, Eric Blake, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 7032 bytes --]
Hi,
On Mon, Aug 29, 2022 at 01:27:06PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > On Fri, Aug 19, 2022 at 11:27:13AM -0500, Andrea Bolognani wrote:
> >> On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
> >> > On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> >> > > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> >> > > > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> >> > > > // Check for json-null first
> >> > > > if string(data) == "null" {
> >> > > > return errors.New(`null not supported for BlockdevRef`)
> >> > > > }
> >> > > > // Check for BlockdevOptions
> >> > > > {
> >> > > > s.Definition = new(BlockdevOptions)
> >> > > > if err := StrictDecode(s.Definition, data); err == nil {
> >> > > > return nil
> >> > > > }
> >> > >
> >> > > The use of StrictDecode() here means that we won't be able to
> >> > > parse an alternate produced by a version of QEMU where
> >> > > BlockdevOptions has gained additional fields, doesn't it?
> >> >
> >> > That's correct. This means that with this RFCv2 proposal, qapi-go
> >> > based on qemu version 7.1 might not be able to decode a qmp
> >> > message from qemu version 7.2 if it has introduced a new field.
> >> >
> >> > This needs fixing, not sure yet the way to go.
> >> >
> >> > > Considering that we will happily parse such a BlockdevOptions
> >> > > outside of the context of BlockdevRef, I think we should be
> >> > > consistent and allow the same to happen here.
> >> >
> >> > StrictDecode is only used with alternates because, unlike unions,
> >> > Alternate types don't have a 'discriminator' field that would
> >> > allow us to know what data type to expect.
> >> >
> >> > With this in mind, theoretically speaking, we could have very
> >> > similar struct types as Alternate fields and we have to find on
> >> > runtime which type is that underlying byte stream.
> >> >
> >> > So, to reply to your suggestion, if we allow BlockdevRef without
> >> > StrictDecode we might find ourselves in a situation that it
> >> > matched a few fields of BlockdevOptions but it the byte stream
> >> > was actually another type.
> >>
> >> IIUC your concern is that the QAPI schema could gain a new
> >> type, TotallyNotBlockdevOptions, which looks exactly like
> >> BlockdevOptions except for one or more extra fields.
> >>
> >> If QEMU then produced a JSON like
> >>
> >> { "description": { /* a TotallyNotBlockdevOptions here */ } }
> >>
> >> and we'd try to deserialize it with Go code like
> >>
> >> ref := BlockdevRef{}
> >> json.Unmarsal(&ref)
> >>
> >> we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
> >> valid BlockdevOptions, dropping the extra fields in the process.
> >>
> >> Does that correctly describe the reason why you feel that the use of
> >> StrictDecode is necessary?
> >
> > Not quite. The problem here is related to the Alternate types of
> > the QAPI specification [0], just to name a simple in-use example,
> > BlockdevRefOrNul [1].
> >
> > [0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L387
> > [1] https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/block-core.json#L4349
> >
> > To exemplify the problem that I try to solve with StrictDecode,
> > let's say there is a DeviceRef alternate type that looks like:
> >
> > { 'alternate': 'DeviceRef',
> > 'data': { 'memory': 'BlockdevRefInMemory',
> > 'disk': 'BlockdevRefInDisk',
> > 'cloud': 'BlockdevRefInCloud' } }
> >
> > Just a quick recap, at runtime we don't have data's payload name
> > (e.g: disk). We need to check the actual data and try to find
> > what is the payload type.
> >
> > type BlockdevRefInMemory struct {
> > Name *string
> > Size uint64
> > Start uint64
> > End uint64
> > }
> >
> > type BlockdevRefInDisk struct {
> > Name *string
> > Size uint64
> > Path *string
> > }
> >
> > type BlockdevRefInCloud struct {
> > Name *string
> > Size uint64
> > Uri *string
> > }
> >
> > All types have unique fields but they all share some fields too.
>
> Quick intercession (I merely skimmed the review thread; forgive me if
> it's not useful or not new):
>
> An alternate type is like a union type, except there is no
> discriminator on the wire. Instead, the branch to use is inferred
> from the value. An alternate can only express a choice between types
> represented differently on the wire.
>
> This is docs/devel/qapi-code-gen.rst. Implied there: the inference is
> based on the JSON type *only*, i.e. no two branches can have the same
> JSON type on the wire. Since all complex types (struct or union) are
> JSON object on the wire, at most one alternate branch can be of complex
> type.
Ah, I've missed this bit. Thank you, it does make it much
simpler.
> More sophisticated inference would be possible if we need it.
> So far we haven't.
Ack.
Cheers,
Victor
> > Golang, without StrictDecode would happily decode a "disk"
> > payload into either "memory" or "cloud" fields, matching only
> > what the json provides and ignoring the rest. StrictDecode would
> > error if the payload had fields that don't belong to that Type so
> > we could try to find a perfect match.
> >
> > While this should work reliably with current version of QEMU's
> > qapi-schema.json, it is not future error proof... It is just a
> > bit better than not checking at all.
> >
> > The overall expectations is that, if the fields have too much in
> > common, it is likely they should have been tagged as 'union'
> > instead of 'alternate'. Yet, because alternate types have this
> > flexibility, we need to be extra careful.
> >
> > I'm still thinking about this in a way that would not give too
> > much hassle when considering a generated code that talks with
> > older/newer qemu where some fields might have been added/removed.
> >
> >> If so, I respectfully disagree :)
> >>
> >> If the client code is expecting a BlockdevRef as the return
> >> value of a command and QEMU is producing something that is
> >> *not* a BlockdevRef instead, that's an obvious bug in QEMU. If
> >> the client code is expecting a BlockdevRef as the return value
> >> of a command that is specified *not* to return a BlockdevRef,
> >> that's an obvious bug in the client code.
> >>
> >> In neither case it should be the responsibility of the SDK to
> >> second-guess the declared intent, especially when it's
> >> perfectly valid for a type to be extended in a
> >> backwards-compatible way by adding fields to it.
> >
> > Yes, the SDK should consider valid QMP messages. This specific
> > case is just a bit more complex qapi type that SDK needs to
> > worry.
> >
> > Thanks for your input!
> > Victor
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
2022-08-29 11:53 ` Markus Armbruster
@ 2022-08-29 14:05 ` Victor Toso
0 siblings, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-08-29 14:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Andrea Bolognani, qemu-devel, Eric Blake, John Snow,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 5422 bytes --]
Hi,
On Mon, Aug 29, 2022 at 01:53:51PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > On Tue, Jul 05, 2022 at 08:46:34AM -0700, Andrea Bolognani wrote:
> >> I've commented in detail to the single patches, just a couple of
> >> additional points.
> >>
> >> On Fri, Jun 17, 2022 at 02:19:24PM +0200, Victor Toso wrote:
> >> > * 7) Flat structs by removing embed types. Discussion with Andrea
> >> > Thread: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
> >> >
> >> > No one required it but I decided to give it a try. Major issue that
> >> > I see with this approach is to have generated a few 'Base' structs
> >> > that are now useless. Overall, less nested structs seems better to
> >> > me. Opnions?
> >> >
> >> > Example:
> >> > | /* This is now useless, should be removed? */
> >> > | type InetSocketAddressBase struct {
> >> > | Host string `json:"host"`
> >> > | Port string `json:"port"`
> >> > | }
> >>
> >> Can we somehow keep track, in the generator, of types that are
> >> only used as building blocks for other types, and prevent them
> >> from showing up in the generated code?
> >
> > I'm not 100% sure it is good to remove them from generated code
> > because technically it is a valid qapi type. If all @base types
> > are embed types and they don't show in other way or form, sure we
> > can remove them from generated code... I'm not sure if it is
> > possible to guarantee this.
> >
> > But yes, if possible, I'd like to remove what seems useless type
> > definitions.
>
> The existing C generators have to generate all the types, because the
> generated code is for QEMU's own use, where we need all the types.
>
> The existing introspection generator generates only the types visible in
> QAPI/QMP introspection.
>
> The former generate for internal use (where we want all the types), and
> the latter for external use (where only the types visible in the
> external interface are actually useful).
My doubt are on types that might be okay to be hidden because
they are embed in other types, like InetSocketAddressBase.
Note that what I mean with the struct being embed is that the
actual fields of InetSocketAddressBase being added to the type
which uses it, like InetSocketAddress.
| type InetSocketAddressBase struct {
| Host string `json:"host"`
| Port string `json:"port"`
| }
|
| type InetSocketAddress struct {
| // Base fields
| Host string `json:"host"`
| Port string `json:"port"`
|
| Numeric *bool `json:"numeric,omitempty"`
| To *uint16 `json:"to,omitempty"`
| Ipv4 *bool `json:"ipv4,omitempty"`
| Ipv6 *bool `json:"ipv6,omitempty"`
| KeepAlive *bool `json:"keep-alive,omitempty"`
| Mptcp *bool `json:"mptcp,omitempty"`
| }
Andrea's suggestions is to have the generator to track if a given
type is always embed in which case we can skip generating it in
the Go module.
I think that could work indeed. In the hypothetical case that
hiddens structs like InetSocketAddressBase becomes a parameter on
command in the future, the generator would know and start
generating this type from that point onwards.
> >> Finally, looking at the repository containing the generated
> >> code I see that the generated type are sorted by kind, e.g. all
> >> unions are in a file, all events in another one and so on. I
> >> believe the structure should match more closely that of the
> >> QAPI schema, so e.g. block-related types should all go in one
> >> file, net-related types in another one and so on.
> >
> > That's something I don't mind adding but some hardcoded mapping
> > is needed. If you look into git history of qapi/ folder, .json
> > files can come and go, types be moved around, etc. So, we need to
> > proper map types in a way that the generated code would be kept
> > stable even if qapi files would have been rearranged. What I
> > proposed was only the simplest solution.
> >
> > Also, the generator takes a qapi-schema.json as input. We are
> > more focused in qemu/qapi/qapi-schema.json generated coded but
> > would not hurt to think we could even use it for qemu-guest-agent
> > from qemu/qga/qapi-schema.json -- this to say that the hardcoded
> > mapping needs to take into account non qemu qapi schemas too.
>
> In the beginning, the QAPI schema was monolithic.
> qga/qapi-schema.json still is.
>
> When keeping everything in a single qapi-schema.json became
> unwieldy, we split it into "modules" tied together with a
> simple include directive. Generated code remained monolithic.
> When monolithic generated code became too annoying (touch
> schema, recompile everything), we made it match the module
> structure: code for FOO.json goes into *-FOO.c and *-FOO.h,
> where the *-FOO.h #include the generated headers for the .json
> modules FOO.json includes.
>
> Schema code motion hasn't been much of a problem. Moving from
> FOO.json to one of the modules it includes is transparent.
> Non-transparent moves are relatively rare as long as the split
> into modules actually makes sense.
To be honest, splitting it into different files based on their
module names should not be a problem if we keep them in a single
Go module as do intend to for this generated go code.
So I'll go ahead and split it.
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
2022-06-17 12:19 ` [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate " Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
@ 2022-09-02 14:49 ` Victor Toso
1 sibling, 0 replies; 47+ messages in thread
From: Victor Toso @ 2022-09-02 14:49 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Markus Armbruster, John Snow, Andrea Bolognani,
Daniel P . Berrangé
[-- Attachment #1: Type: text/plain, Size: 5856 bytes --]
Hi,
On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> At this moment, there are 5 alternates in qemu/qapi, they are:
> * BlockDirtyBitmapMergeSource
> * Qcow2OverlapChecks
> * BlockdevRef
> * BlockdevRefOrNull
> * StrOrNull
So, things got a little bit complicated due the fact that
BlockdevRefOrNull and StrOrNull can take JSON NULL value and
that's completely different than an omitted field.
The last reply from Markus in another patch series make this
clear with a good deal of examples too.
https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00113.html
I'll put the struggle that I'm having just in case someone have
suggestions. Let's get an example:
In qemu/qapi/block-core.json:
{ 'struct': 'BlockdevOptionsQcow2',
'base': 'BlockdevOptionsGenericCOWFormat',
'data': { '*lazy-refcounts': 'bool',
'*pass-discard-request': 'bool',
'*pass-discard-snapshot': 'bool',
'*pass-discard-other': 'bool',
'*overlap-check': 'Qcow2OverlapChecks',
'*cache-size': 'int',
'*l2-cache-size': 'int',
'*l2-cache-entry-size': 'int',
'*refcount-cache-size': 'int',
'*cache-clean-interval': 'int',
'*encrypt': 'BlockdevQcow2Encryption',
'*data-file': 'BlockdevRef' } }
Generates in qapi-go/pkg/qapi/structs.go:
type BlockdevOptionsQcow2 struct {
// Base fields
Backing *BlockdevRefOrNull `json:"backing,omitempty"`
LazyRefcounts *bool `json:"lazy-refcounts,omitempty"`
PassDiscardRequest *bool `json:"pass-discard-request,omitempty"`
PassDiscardSnapshot *bool `json:"pass-discard-snapshot,omitempty"`
PassDiscardOther *bool `json:"pass-discard-other,omitempty"`
OverlapCheck *Qcow2OverlapChecks `json:"overlap-check,omitempty"`
CacheSize *int64 `json:"cache-size,omitempty"`
L2CacheSize *int64 `json:"l2-cache-size,omitempty"`
L2CacheEntrySize *int64 `json:"l2-cache-entry-size,omitempty"`
RefcountCacheSize *int64 `json:"refcount-cache-size,omitempty"`
CacheCleanInterval *int64 `json:"cache-clean-interval,omitempty"`
Encrypt *BlockdevQcow2Encryption `json:"encrypt,omitempty"`
DataFile *BlockdevRef `json:"data-file,omitempty"`
}
One thing that we assumed for all types is that an optional type
can be simply ignored. For that to work with Golang's
encoding/json, we make do by making every optional field a
pointer to the type it represents, plus we add the "omitempty"
tag. This way, if the user did not set it, it is simply ignored.
If we didn't receive anything, all should be good.
The problem happens when we receive a JSON Null value, which is
one possible value type for the BlockdevRefOrNull above. A
message like { "backing" : null } does not trigger UnmarshalJSON
for BlockdevRefOrNull and this is silently ignored.
In Go's encoding/json, in order to { "backing" : null } trigger
UnmarshalJSON, we need to remove omitempty and also the pointer
from Backing. Now, on the marshalling side, we will always have
'backing' being set and in order to omit it from the output
(which we might want indeed) we need to rework the byte array,
which is something we were trying to avoid :)
I've looked up and there has been proposals to address this kind
of issue, including an omitnil tag [0] that could be used here to
workaround it but that's unlikely to move forward [1].
The first thing to do is to define when to omit *StrOrNull and
*BlockdevRefOrNull and when to set it to null. The solution I
thought would make sense is to have a Null boolean field that
user would need to set, so:
type BlockdevRefOrNull struct {
Definition *BlockdevOptions
Reference *string
Null bool
}
func (s BlockdevRefOrNull) MarshalJSON() ([]byte, error) {
if s.Definition != nil {
return json.Marshal(s.Definition)
} else if s.Reference != nil {
return json.Marshal(s.Reference)
} else if s.Null {
return []byte("null"), nil
}
// If only QAPI spec accepted "backing": {} as the same
// as ommited.
return []byte("qapi-go-remove-this-object"), nil
}
func (s *BlockdevRefOrNull) UnmarshalJSON(data []byte) error {
// Check for json-null first
if string(data) == "null" {
s.Null = true
return nil
}
// Check for BlockdevOptions
...
}
Setting BlockdevRefOrNull to null and to StrOrNull could have
different meanings which is explained in the documentation
itself. That's why I think Null as a boolean is fine, the user
sets for a specific context when it knows what it is doing...
Not having a pointer for optional fields of this two types breaks
the coherence we had that all optional members are pointers in
Go. This hurts a bit but this two are truly special as they can
have the magic null value.
Now, I'm thinking on a reasonable way to remove the object that
wraps "qapi-go-remove-this-object" as well, likely using the
json's decoder [2]...
[0] https://github.com/golang/go/issues/22480
[1] https://github.com/golang/go/issues/34701#issuecomment-544710311
[2] https://pkg.go.dev/encoding/json#Decoder.Token
Cheers,
Victor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2022-09-02 14:51 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-17 12:19 [RFC PATCH v2 0/8] qapi: add generator for Golang interface Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 1/8] qapi: golang: Generate qapi's enum types in Go Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate " Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-08-17 14:04 ` Victor Toso
2022-08-19 16:27 ` Andrea Bolognani
2022-08-22 6:59 ` Victor Toso
2022-08-29 11:27 ` Markus Armbruster
2022-08-29 13:31 ` Victor Toso
2022-09-02 14:49 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct " Victor Toso
2022-06-17 14:41 ` Daniel P. Berrangé
2022-06-17 15:23 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union " Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-07-05 16:35 ` Daniel P. Berrangé
2022-07-06 9:28 ` Andrea Bolognani
2022-07-06 9:37 ` Daniel P. Berrangé
2022-07-06 9:48 ` Daniel P. Berrangé
2022-07-06 12:20 ` Andrea Bolognani
2022-08-17 16:25 ` Victor Toso
2022-08-19 7:20 ` Victor Toso
2022-08-19 15:25 ` Andrea Bolognani
2022-08-22 6:33 ` Victor Toso
2022-08-17 16:06 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event " Victor Toso
2022-07-05 15:45 ` Andrea Bolognani
2022-07-05 16:47 ` Daniel P. Berrangé
2022-07-06 14:53 ` Andrea Bolognani
2022-07-06 15:07 ` Daniel P. Berrangé
2022-07-06 16:22 ` Andrea Bolognani
2022-08-18 7:55 ` Victor Toso
2022-08-18 7:47 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 6/8] qapi: golang: Generate qapi's command " Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go Victor Toso
2022-07-05 15:46 ` Andrea Bolognani
2022-07-05 16:49 ` Daniel P. Berrangé
2022-08-17 15:00 ` Victor Toso
2022-06-17 12:19 ` [RFC PATCH v2 8/8] qapi: golang: document skip function visit_array_types Victor Toso
2022-06-27 7:15 ` [RFC PATCH v2 0/8] qapi: add generator for Golang interface Markus Armbruster
2022-06-27 12:48 ` Victor Toso
2022-06-27 15:29 ` Markus Armbruster
2022-08-18 8:04 ` Victor Toso
2022-07-05 15:46 ` Andrea Bolognani
2022-08-17 14:24 ` Victor Toso
2022-08-29 11:53 ` Markus Armbruster
2022-08-29 14:05 ` Victor Toso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.