Hi,
On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > > All this string manipulation looks sketchy. Is there some reason that
> > > I'm not seeing preventing you for doing something like the untested
> > > code below?
> > >
> > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > if s.HyperV != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > > }
> > > tmp := union {
> > > Discriminator: "hyper-v",
> > > HyperV: s.HyperV,
> > > }
> > > return json.Marshal(tmp)
> > > } else if s.S390 != nil {
> > > type union struct {
> > > Discriminator string `json:"type"`
> > > S390 GuestPanicInformationHyperV `json:"s390"`
> > > }
> > > tmp := union {
> > > Discriminator: "s390",
> > > S390: s.S390,
> > > }
> > > return json.Marshal(tmp)
> > > }
> > > return nil, errors.New("...")
> > > }
> >
> > Using these dummy structs is the way I've approached the
> > discriminated union issue in the libvirt Golang XML bindings
> > and it works well. It is the bit I like the least, but it was
> > the lesser of many evils, and on the plus side in the QEMU case
> > it'll be auto-generated code.
>
> It appears to be the standard way to approach the problem in Go. It
> sort of comes naturally given how the APIs for marshal/unmarshal have
> been defined.
Yep, string manipulation was bad choice. Some sort of anonymous
struct is a better fit. So I'll be changing this ...
> > > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > > type Alias GuestPanicInformation
> > > > peek := struct {
> > > > Alias
> > > > Driver string `json:"type"`
> > > > }{}
> > > >
> > > > if err := json.Unmarshal(data, &peek); err != nil {
> > > > return err
> > > > }
> > > > *s = GuestPanicInformation(peek.Alias)
> > > >
> > > > switch peek.Driver {
> > > >
> > > > case "hyper-v":
> > > > s.HyperV = new(GuestPanicInformationHyperV)
> > > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > > s.HyperV = nil
> > > > return err
> > > > }
> > > > case "s390":
> > > > s.S390 = new(GuestPanicInformationS390)
> > > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > > s.S390 = nil
> > > > return err
> > > > }
> > > > }
> > > > // Unrecognizer drivers are silently ignored.
> > > > return nil
> > >
> > > This looks pretty reasonable, but since you're only using "peek" to
> > > look at the discriminator you should be able to leave out the Alias
> > > type entirely and perform the initial Unmarshal operation while
> > > ignoring all other fields.
> >
> > Once you've defined the dummy structs for the Marshall case
> > though, you might as well use them for Unmarshall too, so you're
> > not parsing the JSON twice.
>
> You're right, that is undesirable. What about something like this?
>
> type GuestPanicInformation struct {
> HyperV *GuestPanicInformationHyperV
> S390 *GuestPanicInformationS390
> }
>
> type jsonGuestPanicInformation struct {
> Discriminator string `json:"type"`
> HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> S390 *GuestPanicInformationS390 `json:"s390"`
> }
I didn't test this so I could be wrong but, I think this should
not work in case you want to remove the string manipulation.
The marshalling of either HyperV or S390 fields would return a
JSON Object but QAPI spec expects the fields at the same level as
the discriminator's type [0]. So, with this you still need some
string manipulation to remove the extra {}, like I did poorly
without any comment 0:-)
[0]
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L358
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> if (s.HyperV != nil && s.S390 != nil) ||
> (s.HyperV == nil && s.S390 == nil) {
> // client hasn't filled in the struct properly
> return nil, errors.New("...")
> }
>
> tmp := jsonGuestPanicInformation{}
>
> if s.HyperV != nil {
> tmp.Discriminator = "hyper-v"
> tmp.HyperV = s.HyperV
> } else if s.S390 != nil {
> tmp.Discriminator = "s390"
> tmp.S390 = s.S390
> }
>
> return json.Marshal(tmp)
> }
>
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> tmp := jsonGuestPanicInformation{}
>
> err := json.Unmarshal(data, &tmp)
> if err != nil {
> return err
> }
>
> switch tmp.Discriminator {
> case "hyper-v":
> if tmp.HyperV == nil {
> return errors.New("...")
> }
> s.HyperV = tmp.HyperV
> case "s390":
> if tmp.S390 == nil {
> return errors.New("...")
> }
> s.S390 = tmp.S390
> }
> // if we hit none of the cases above, that means the
> // server has produced a variant we don't know about
>
> return nil
> }
>
> This avoid parsing the JSON twice as well as having to define
> multiple dummy structs, which keeps the code shorter and more
> readable.
If not too verbose, I'd still use anonymous structs whenever
possible. They are created for a given scope, they are basically
self documented in that given context and don't polluted
package's namespace.
> I've also thrown in some additional error checking for good
> measure, ensuring that we abort when the input is completely
> nonsensical from a semantical standpoint.
Thanks!
Victor
signature.asc
Description: PGP signature
