Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
On 3/2/20 8:10 PM, Nick Rosbrook wrote: > Generate constructors for generated Go types. Call libxl__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 Reviewed-by: George Dunlap And checked in, thanks. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
> 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
> Frontendstring
> FrontendId uint32
> Devid Devid
> State intWhich means if libxl passes back
> Evtch int
> Rrefint
> 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.
Oh, yeah I see the problem in xenlight_golang_union_from_C -- it just
continues in the loop if f.type is None to avoid defining another
fromC, but should still should add a case in the switch statement.
Good catch, thanks.
-NR
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
> Although, I'm not sure if that implies "There's already boilerplate, so > it's extra important to avoid adding more", or "There's already > boilerplate, so it won't hurt to have a bit more, and wrap the whole > thing in a nicer library." I think the boilerplate added here is necessary. We need to provide a uniform way for users to initialize a xenlight type which hides the use of _init() and fromC, and I think this is the simplest way to do that. And, I'm not opposed to the idea of writing a "nicer" package to wrap xenlight. > OTOH, we should be able to have libxl automatically copy c_info.type > from b_info.type if c_info.type == LIBXL_DOMAIN_TYPE_INVALID -- if it > doesn't do so already. That sounds simple enough. -NR ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
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__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
>
> 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
Frontendstring
FrontendId uint32
Devid Devid
State intWhich means if libxl passes back
Evtch int
Rrefint
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
On 3/2/20 8:10 PM, Nick Rosbrook wrote:
> Generate constructors for generated Go types. Call libxl__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
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(xl.DomainTypePv)
if err != nil {
fmt.Printf("NewDomainConfig: %v\n", err)
return
}
dconf.BInfo = xl.NewDomainBuildInfo(xl.DomainTypePv)
I've already got to do:
dconf.CInfo.Type = xl.DomainTypePv
Although, I'm not sure if that implies "There's already boilerplate, so
it's extra important to avoid adding more", or "There's already
boilerplate, so it won't hurt to have a bit more, and wrap the whole
thing in a nicer library."
OTOH, we should be able to have libxl automatically copy c_info.type
from b_info.type if c_info.type == LIBXL_DOMAIN_TYPE_INVALID -- if it
doesn't do so already.
Thoughts?
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] golang/xenlight: implement constructor generation
> Generate constructors for generated Go types. Call libxl__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 Note that this is really a resend, but after I sent this patch the first time I realized I forgot to add the dispose() calls. So, I marked this as v2 with that change. Thanks, NR ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
