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

[Victor Toso]

Hi,

On Tue, May 10, 2022 at 12:19:57PM +0100, Daniel P. Berrangé wrote:
> > 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.

I think it is useful to know that, a binary compiled to qapi-go
7.0 but running with qemu 7.1 would have some mismatches. It
could help detect issues (e.g: Why my program doesn't know/show
the reason for shutdown?).

So, some sort of --verbose option for this level should exist.

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

Ok, thanks.

Cheers,
Victor

signature.asc
Description: PGP signature