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
