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

[Victor Toso]

Hi,

On Tue, May 10, 2022 at 11:06:16AM +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 02, 2022 at 12:40:57AM +0200, Victor Toso wrote:
> > This patch handles QAPI enum types and generates its equivalent in Go.
> > 
> > The highlights of this implementation are:
> > 
> > 1. For each QAPI enum, we will define an int32 type in Go to be the
> >    assigned type of this specific enum
> 
> I think there's a potential case to be made for considering
> representation of enums as strings in Golang, as it more
> gracefully degrades.
> 
> Consider the 'SHUTDOWN' event in QEMU, which has a 'reason' field.
> 
> This implementation has
> 
> type ShutdownCause int32
> 
> const (
>         ShutdownCauseNone               ShutdownCause = iota
>         ShutdownCauseHostError                        // An error prevents 
> further use of guest
>         ShutdownCauseHostQmpQuit                      // Reaction to the QMP 
> command 'quit'
>         ShutdownCauseHostQmpSystemReset               // Reaction to the QMP 
> command 'system_reset'
>         ShutdownCauseHostSignal                       // Reaction to a 
> signal, such as SIGINT
>         ShutdownCauseHostUi                           // Reaction to a UI 
> event, like window close
>         ShutdownCauseGuestShutdown                    // Guest 
> shutdown/suspend request, via ACPI or other hardware-specific means
>         ShutdownCauseGuestReset                       // Guest reset request, 
> and command line turns that into a shutdown
>         ShutdownCauseGuestPanic                       // Guest panicked, and 
> command line turns that into a shutdown
>         ShutdownCauseSubsystemReset                   // Partial guest reset 
> that does not trigger QMP events and ignores --no-reboot. This is useful for 
> sanitizing hypercalls on s390 that are used during kexec/kdump/boot
> )
> 
> and
> 
> type ShutdownEvent struct {
>         Guest  bool          `json:"guest"`  // If true, the shutdown was 
> triggered by a guest request (such as a guest-initiated ACPI shutdown request 
> or other hardware-specific action) rather than a host request (such as 
> sending qemu a SIGINT). (since 2.10)
>         Reason ShutdownCause `json:"reason"` // The @ShutdownCause which 
> resulted in the SHUTDOWN. (since 4.0)
> }
> 
> Consider that the application is compiled against the QAPI
> generated API from QEMU 7.0. The application is believed to run
> happily against 7.1, because app doesn't need any of the
> functionality added in 7.1 QAPI.  But consider that QEMU 7.1
> had introduced a new shutdown reason value.
> 
> The application may want to know that the guest has shutdown,
> but does NOT care about the reason for the shutdown.
> 
> Since the ShutdownReason is implemented as an int though, the
> unmarshalling for the reason field is going to fail.

Marshalling does error if you try to convert an int that is not
in the range of the enum type.

Unmarshalling should not error in this case, but the field ends
up not being set which defaults to 0 (in this case,
ShutdownCauseNone). That could mislead the *actual* reason but
not without a warning which is expected in this case, imho.

(I know is not an actual warning, just a Println, but it'll be
fixed in the next iteration)

  | func (s *ShutdownCause) UnmarshalJSON(data []byte) error {
  |     var name string
  |
  |     if err := json.Unmarshal(data, &name); err != nil {
  |         return err
  |     }
  |
  |     switch name {
  |     case "none":
  |         (*s) = ShutdownCauseNone
  |     case "host-error":
  |         (*s) = ShutdownCauseHostError
  |     case "host-qmp-quit":
  |         (*s) = ShutdownCauseHostQmpQuit
  |     case "host-qmp-system-reset":
  |         (*s) = ShutdownCauseHostQmpSystemReset
  |     case "host-signal":
  |         (*s) = ShutdownCauseHostSignal
  |     case "host-ui":
  |         (*s) = ShutdownCauseHostUi
  |     case "guest-shutdown":
  |         (*s) = ShutdownCauseGuestShutdown
  |     case "guest-reset":
  |         (*s) = ShutdownCauseGuestReset
  |     case "guest-panic":
  |         (*s) = ShutdownCauseGuestPanic
  |     case "subsystem-reset":
  |         (*s) = ShutdownCauseSubsystemReset
  |     default:
  |         fmt.Println("Failed to decode ShutdownCause", *s)
  |     }
  |     return nil
  | }

> If the enums were just represented as strings, then we can
> gracefully accept any new enum value that arrives in future.
> The application can thus also still log the shutdown reason
> string, even though it was not a value explicitly known to the
> generated API.

As a string, the warning should still exist and default value of
"" or nil for ptr would apply. IMHO, between string + warning and
int + warning, I'd still go with int here.

That's a design decision to be made. All in all, I don't know
much about the changes in QEMU/QAPI per version to take this
decision, so I'll rely on you and the list for this, not just for
enums but for the other types too.

> > 2. While in the Go codebase we can use the generated enum values, the
> >    specification requires that, on the wire, the enumeration type's
> >    value to be represented by its string name.
> > 
> >    For this reason, each Go type that represent's a QAPI enum will be
> >    implementing the Marshaler[0] and Unmarshaler[1] interfaces to
> >    seamless handle QMP's string to Go int32 value and vice-versa.
> > 
> > 3. Naming: CamelCase will be used in any identifier that we want to
> >    export [2], which is everything in this patch.

Cheers,
Victor

signature.asc
Description: PGP signature