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

[Victor Toso]

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

signature.asc
Description: PGP signature