On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> Alternate types are similar to Union but without a discriminator that
> can be used to identify the underlying value on the wire. It is needed
> to infer it. In Go, most of the types [*] are mapped as optional
> fields and Marshal and Unmarshal methods will be handling the data
> checks.
>
> Example:
>
> qapi:
> | { 'alternate': 'BlockdevRef',
> | 'data': { 'definition': 'BlockdevOptions',
> | 'reference': 'str' } }
>
> go:
> | type BlockdevRef struct {
> | Definition *BlockdevOptions
> | Reference *string
> | }
>
> usage:
> | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
> | k := BlockdevRef{}
> | err := json.Unmarshal([]byte(input), &k)
> | if err != nil {
> | panic(err)
> | }
> | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
>
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go. This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.
These usage examples are great; in fact, I think they're too good to
be relegated to the commit messages. I would like to see them in the
actual documentation.
At the same time, the current documentation seems to focus a lot on
internals rather than usage. I think we really need two documents
here:
* one for users of the library, with lots of usage examples
(ideally at least one for JSON->Go and one for Go->JSON for each
class of QAPI object) and little-to-no peeking behind the
curtains;
* one for QEMU developers / QAPI maintainers, which goes into
detail regarding the internals and explains the various design
choices and trade-offs.
Some parts of the latter should probably be code comments instead. A
reasonable balance will have to be found.
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> +TEMPLATE_HELPER = """
> +// Creates a decoder that errors on unknown Fields
> +// Returns nil if successfully decoded @from payload to @into type
> +// Returns error if failed to decode @from payload to @into type
> +func StrictDecode(into interface{}, from []byte) error {
> +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> +\tdec.DisallowUnknownFields()
> +
> +\tif err := dec.Decode(into); err != nil {
> +\t\treturn err
> +\t}
> +\treturn nil
> +}
> +"""
I think the use of \t here makes things a lot less readable. Can't
you just do
TEMPLATE_HELPER = """
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
I would actually recommend the use of textwrap.dedent() to make
things less awkward:
TEMPLATE_HELPER = textwrap.dedent("""
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
This is particularly useful for blocks of Go code that are not
declared at the top level...
> + unmarshal_check_fields = f"""
> +\t// Check for json-null first
> +\tif string(data) == "null" {{
> +\t\treturn errors.New(`null not supported for {name}`)
> +\t}}"""
... such as this one, which could be turned into:
unmarshal_check_fields = textwrap.dedent(f"""
// Check for json-null first
if string(data) == "null" {{
return errors.New(`null not supported for {name}`)
}}
""")
Much more manageable, don't you think? :)
On a partially related note: while I haven't yet looked closely at
how much effort you've dedicated to producing pretty output, from a
quick look at generate_struct_type() it seems that the answer is "not
zero". I think it would be fine to simplify things there and produce
ugly output, under the assumption that gofmt will be called on the
generated code immediately afterwards. The C generator doesn't have
this luxury, but we should take advantage of it.
--
Andrea Bolognani / Red Hat / Virtualization
