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

[Daniel P . Berrangé]

On Sat, Apr 02, 2022 at 12:41:01AM +0200, Victor Toso wrote:
> This patch handles QAPI event types and generates data structures in
> Go that handles it.
> 
> At the moment of this writing, it generates 51 structures (49 events)
> 
> In Golang, each event is handled as a Go structure and there is no big
> difference, in the Go generated code, between what is a QAPI event
> type and what is a QAPI struct.
> 
> Each QAPI event has the suffix 'Event' in its Golang data structure
> and contains the fields, mandatory and optional, that can be
> sent or received.
> 
> In addition, there are two structures added to handle QAPI
> specification for event types: 'Event' and 'EventBase'.
> 
> 'EventBase' contains @Name and @Timestamp fields and then 'Event'
> extends 'EventBase' with an @Arg field of type 'Any'.

Again, I don't think we should need to use an Any type here.

Rather than 

  type EventBase struct {
        Name      string `json:"event"`
        Timestamp struct {
                Seconds      int64 `json:"seconds"`
                Microseconds int64 `json:"microseconds"`
        } `json:"timestamp"`
  }

  type Event struct {
        EventBase
        Arg Any `json:"data,omitempty"`
  }

  type ShutdownEvent struct {
        Guest  bool          `json:"guest"`
        Reason ShutdownCause `json:"reason"`
  }


I think we should just embed EventBase directly in each specific
event eg


  type Event struct {
        Name      string `json:"event"`
        Timestamp struct {
                Seconds      int64 `json:"seconds"`
                Microseconds int64 `json:"microseconds"`
        } `json:"timestamp"`
  }

  type ShutdownEvent struct {
        Event Event
        Guest  bool          `json:"guest"`
        Reason ShutdownCause `json:"reason"`
  }


Or perhaps better still, use an interface 

  type Event interface {
      GetName() string
      GetTimestamp() string
  }

  type Timestamp struct {
      Seconds      int64 `json:"seconds"`
      Microseconds int64 `json:"microseconds"`
  }

  type ShutdownEvent struct {
        Timestamp Timestamp  `json:"timestamp"`
        Guest  bool          `json:"guest"`
        Reason ShutdownCause `json:"reason"`
  }

  func (ev *ShutdownEvent) GetName() string {
        return "SHUTDOWN"
  }

That way you can define public APIs taking 'Event' as a type,
and impls of the events can be passed directly in/out.

Similar comment for the Command type.

> 
> The 'Event' type implements the Unmarshaler to decode the QMP JSON
> Object into the correct Golang (event) struct. The goal here is to
> facilitate receiving Events.
> 
> A TODO for this type is to implement Marshaler for 'Event'. It'll
> containt runtime checks to validate before transforming the struct
> into a JSON Object.
> 
> Example:
> ```go
>     qmpMsg := `{
>     "event" : "MIGRATION",
>     "timestamp":{
>         "seconds": 1432121972,
>         "microseconds": 744001
>     },
>     "data":{
>         "status": "completed"
>     }
> }`
> 
>     e := Event{}
>     _ = json.Unmarshal([]byte(qmpMsg), &e)
>     // e.Name == "MIGRATION"
>     // e.Arg.(MigrationEvent).Status == MigrationStatusCompleted
> ```
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  scripts/qapi/golang.py | 92 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> index 0a1bf430ba..3bb66d07c7 100644
> --- a/scripts/qapi/golang.py
> +++ b/scripts/qapi/golang.py
> @@ -31,9 +31,10 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
>  
>      def __init__(self, prefix: str):
>          super().__init__()
> -        self.target = {name: "" for name in ["alternate", "enum", "helper", 
> "struct", "union"]}
> +        self.target = {name: "" for name in ["alternate", "enum", "event", 
> "helper", "struct", "union"]}
>          self.objects_seen = {}
>          self.schema = None
> +        self.events = {}
>          self._docmap = {}
>          self.golang_package_name = "qapi"
>  
> @@ -57,6 +58,24 @@ def visit_begin(self, schema):
>      def visit_end(self):
>          self.schema = None
>  
> +        # EventBase and Event are not specified in the QAPI schema,
> +        # so we need to generate it ourselves.
> +        self.target["event"] += '''
> +type EventBase struct {
> +    Name      string `json:"event"`
> +    Timestamp struct {
> +        Seconds      int64 `json:"seconds"`
> +        Microseconds int64 `json:"microseconds"`
> +    } `json:"timestamp"`
> +}
> +
> +type Event struct {
> +    EventBase
> +    Arg       Any    `json:"data,omitempty"`
> +}
> +'''
> +        self.target["event"] += generate_marshal_methods('Event', 
> self.events)
> +
>          self.target["helper"] += '''
>  // Creates a decoder that errors on unknown Fields
>  // Returns true if successfully decoded @from string @into type
> @@ -279,7 +298,28 @@ 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
> +
> +        doc = self._docmap.get(name, None)
> +        self_contained = True if not arg_type or not 
> arg_type.name.startswith("q_obj") else False
> +        content = ""
> +        if self_contained:
> +            doc_struct, _ = qapi_to_golang_struct_docs(doc)
> +            content = generate_struct_type(type_name, "", doc_struct)
> +        else:
> +            assert isinstance(arg_type, QAPISchemaObjectType)
> +            content = qapi_to_golang_struct(name,
> +                                            doc,
> +                                            arg_type.info,
> +                                            arg_type.ifcond,
> +                                            arg_type.features,
> +                                            arg_type.base,
> +                                            arg_type.members,
> +                                            arg_type.variants)
> +
> +        self.target["event"] += content
>  
>      def write(self, output_dir: str) -> None:
>          for module_name, content in self.target.items():
> @@ -351,15 +391,41 @@ def generate_marshal_methods_enum(members: 
> List[QAPISchemaEnumMember]) -> str:
>  }}
>  '''
>  
> -# Marshal methods for Union types
> +# Marshal methods for Event and Union types
>  def generate_marshal_methods(type: str,
>                               type_dict: Dict[str, str],
>                               discriminator: str = "",
>                               base: str = "") -> str:
> -    assert base != ""
> -    discriminator = "base." + discriminator
> -
> -    switch_case_format = '''
> +    type_is_union = False
> +    json_field = ""
> +    struct_field = ""
> +    if type == "Event":
> +        base = type + "Base"
> +        discriminator = "base.Name"
> +        struct_field = "Arg"
> +        json_field = "data"
> +    else:
> +        assert base != ""
> +        discriminator = "base." + discriminator
> +        type_is_union = True
> +
> +    switch_case_format = ""
> +    if not type_is_union:
> +        switch_case_format = '''
> +    case "{name}":
> +        tmp := struct {{
> +            Value {isptr}{case_type} `json:"{json_field},omitempty"`
> +        }}{{}}
> +        if err := json.Unmarshal(data, &tmp); err != nil {{
> +            return err
> +        }}
> +        if tmp.Value == nil {{
> +            s.{struct_field} = nil
> +        }} else {{
> +            s.{struct_field} = {isptr}tmp.Value
> +        }}'''
> +    else:
> +        switch_case_format = '''
>      case {name}:
>          value := {case_type}{{}}
>          if err := json.Unmarshal(data, &value); err != nil {{
> @@ -374,12 +440,17 @@ def generate_marshal_methods(type: str,
>          case_type = type_dict[name]
>          isptr = "*" if case_type[0] not in "*[" else ""
>          switch_cases += switch_case_format.format(name = name,
> +                                                  struct_field = 
> struct_field,
> +                                                  json_field = json_field,
> +                                                  isptr = isptr,
>                                                    case_type = case_type)
>          if case_type not in added:
>              if_supported_types += f'''typestr != "{case_type}" &&\n\t\t'''
>              added[case_type] = True
>  
> -    marshalfn = f'''
> +    marshalfn = ""
> +    if type_is_union:
> +        marshalfn = f'''
>  func (s {type}) MarshalJSON() ([]byte, error) {{
>       base, err := json.Marshal(s.{base})
>       if err != nil {{
> @@ -564,4 +635,9 @@ def qapi_to_go_type_name(name: str, meta: str) -> str:
>      words = [word for word in name.replace("_", "-").split("-")]
>      name = words[0].title() if words[0].islower() or words[0].isupper() else 
> words[0]
>      name += ''.join(word.title() for word in words[1:])
> +
> +    if meta == "event":
> +        name = name[:-3] if name.endswith("Arg") else name
> +        name += meta.title()
> +
>      return name
> -- 
> 2.35.1
> 
> 

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 :|