Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 3/4/20 6:08 PM, George Dunlap wrote:
> On 3/2/20 8:10 PM, Nick Rosbrook wrote:
>> Generate constructors for generated Go types. Call libxl_<type>_init so
>> the Go type can be properly initialized.
>>
>> If a type has a keyed union field, add a parameter to the function
>> signature to set the key variable, and call the init function for the
>> keyed union.>
>> Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> 
> So gave this a spin and ran a cross a niggle...
> 
>> +// NewDomainBuildInfo returns an instance of DomainBuildInfo initialized 
>> with defaults.
>> +func NewDomainBuildInfo(dtype DomainType) (*DomainBuildInfo, error) {
> 
> NewDomainBuildInfo() will take the domain type; but what I really want is...
> 
>> +// NewDomainConfig returns an instance of DomainConfig initialized with 
>> defaults.
>> +func NewDomainConfig() (*DomainConfig, error) {
> 
> ...for NewDomainConfig() to take the Domain Type.  Otherwise I'm in a
> position of having to do something like:
> 
>       dconf, err := xl.NewDomainConfig()
>       if err != nil {
>               fmt.Printf("NewDomainConfig: %v\n", err)
>               return
>       }
>         dconf.BInfo = xl.NewDomainBuildInfo(xl.DomainTypePv)

NewDomainConfig() as of this patch can never return success, because
DomainConfig.fromC() will call DomainBuildInfo.fromC(), which will choke
on b_info.type = LIBXL_DOMAIN_TYPE_INVALID.

This is actually a bug in to/fromC.  Consider libxl_channelinfo.

The idl says:

libxl_channelinfo = Struct("channelinfo", [
    ("backend", string),
    ("backend_id", uint32),
    ("frontend", string),
    ("frontend_id", uint32),
    ("devid", libxl_devid),
    ("state", integer),
    ("evtch", integer),
    ("rref", integer),
    ("u", KeyedUnion(None, libxl_channel_connection, "connection",
           [("unknown", None),
            ("pty", Struct(None, [("path", string),])),
            ("socket", None),
           ])),
    ], dir=DIR_OUT)

But the generated code currently only generates:

type Channelinfo struct {
        Backend         string
        BackendId       uint32
        Frontend        string
        FrontendId      uint32
        Devid           Devid
        State           intWhich means if libxl passes back
        Evtch           int
        Rref            int
        Connection      ChannelConnection
        ConnectionUnion channelinfoConnectionUnion
}

type channelinfoConnectionUnion interface {
        ischannelinfoConnectionUnion()
}

type ChannelinfoConnectionUnionPty struct {
        Path string
}

func (x ChannelinfoConnectionUnionPty) ischannelinfoConnectionUnion() {}

I think this makes sense -- there's no need to have types for 'unknown'
and 'socket' just to hold nothing.  But then the marshaling code looks
like this:

        switch x.Connection {
        case ChannelConnectionPty:
                tmp, ok := x.ConnectionUnion.(ChannelinfoConnectionUnionPty)
                if !ok {
                        return errors.New("wrong type for union key connection")
                }
                var pty C.libxl_channelinfo_connection_union_pty
                if tmp.Path != "" {
                        pty.path = C.CString(tmp.Path)
                }
                ptyBytes := C.GoBytes(unsafe.Pointer(&pty),
C.sizeof_libxl_channelinfo_connection_union_pty)
                copy(xc.u[:], ptyBytes)
        default:
                return fmt.Errorf("invalid union key '%v'", x.Connection)
        }

So this will incorrectly fail for for either 'unknown' or 'socket'.
What we need to have is for toC to ignore enumerated values that have
empty types, and fromC to set the union to `nil` in these cases.

I've got a patch -- I'll send it out.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel