Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go

[Victor Toso]

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

signature.asc
Description: PGP signature