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

[Daniel P . Berrangé]

On Tue, May 10, 2022 at 01:15:32PM +0200, Victor Toso wrote:
> 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)

I don't thinnk we should be emitting warnings/printlns at all
in this case though. The app should be able to consume the
enum without needing a warning.  If the app wants to validate
against a specific set of constants, it can decide to emit
a warning if there was a case it can't handle. We shouldn't
emit warnings/errors in the unmarshalling step though,as it
isn't the right place to decide on such policy.

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

Essentially every release of QEMU will have QAPI changes. Most of
the time these are append-only changes. ie a new struct, new command,
new field, new enum value.  Sometimes there will be removals due
to deprecation, though note my other reply saying that we really
ought to stop doing removals from the schema, and instead just
annotate when a field stops being used.

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