Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go

[Victor Toso]

Hi,

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

Didn't know this one, I agree 100% that is nicer. I'll take a
look for the next iteration if the output would still be similar
as I want to gofmt change be zero (see bellow).
 
> 
> On a partially related note: while I haven't yet looked closely at
> how much effort you've dedicated to producing pretty output, from a
> quick look at generate_struct_type() it seems that the answer is "not
> zero". I think it would be fine to simplify things there and produce
> ugly output, under the assumption that gofmt will be called on the
> generated code immediately afterwards. The C generator doesn't have
> this luxury, but we should take advantage of it.

Yes, I wholeheartedly agree. The idea of the generator producing
a well formatted Go code came from previous review. I didn't want
to introduce gofmt and friends to QEMU build system, perhaps it
wasn't a big deal but I find it foreign to QEMU for a generated
code that QEMU itself would not use.

See: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01188.html

Cheers,
Victor

signature.asc
Description: PGP signature