| Message ID | 5773984ae9308500183adde21cf25837bba39f7f.1570456846.git.rosbrookn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | generated Go libxl bindings using IDL | expand |
On 10/7/19 4:12 PM, Nick Rosbrook wrote: > From: Nick Rosbrook <rosbrookn@ainfosec.com> > > Define CpuidPolicyList as a wrapper struct with field val of type > *C.libxl_cpuid_policy_list and implement fromC and toC functions. > > Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com> > --- > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wl@xen.org> > > tools/golang/xenlight/xenlight.go | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go > index d41de253f3..9c384485e1 100644 > --- a/tools/golang/xenlight/xenlight.go > +++ b/tools/golang/xenlight/xenlight.go > @@ -249,6 +249,26 @@ type EvLink struct{} > func (el *EvLink) fromC(cel *C.libxl_ev_link) error { return nil } > func (el *EvLink) toC() (cel C.libxl_ev_link, err error) { return } > > +// CpuidPolicyList represents a libxl_cpuid_policy_list. > +type CpuidPolicyList struct { > + val *C.libxl_cpuid_policy_list > +} Hmm, this introduces a pretty significant risk of memory leaks; but I don't really see any way around it. I guess we really want to do some SetFinalizer() magic on this to call libxl_cpuid_dispose()? We might also want to add something like a .Dispose() method to have predictable memory effects. But then do we want to have a .Dispose() method on all types that might contain a CpuidPolicyList? Technically we're supposed to, so we might have to. (And now I'm having deja vu, like we've had this discussion before, but I can't seem to find it.) -George
> Hmm, this introduces a pretty significant risk of memory leaks; but I > don't really see any way around it. I guess we really want to do some > SetFinalizer() magic on this to call libxl_cpuid_dispose()? > > We might also want to add something like a .Dispose() method to have > predictable memory effects. But then do we want to have a .Dispose() > method on all types that might contain a CpuidPolicyList? Technically > we're supposed to, so we might have to. (And now I'm having deja vu, > like we've had this discussion before, but I can't seem to find it.) As I've expressed before, I don't think its a good idea to look to the runtime to fix this sort of problem, so I'd be more inclined to look into a Dispose like option. But then it does seem weird from an API perspective to only define Dispose on some types since it introduces a closer, but incomplete, semantic relationship between libxl and the Go package. WRT the definition of CpuidPolicyList, is the best we can do? Or is there a way we can hide the use of the C type better so that someone using this package doesn't need to worry about calling Dispose or otherwise? I think [1] is where we originally discussed this. -NR [1] https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg01112.html
On 11/14/19 2:58 PM, Nick Rosbrook wrote: >> Hmm, this introduces a pretty significant risk of memory leaks; but I >> don't really see any way around it. I guess we really want to do some >> SetFinalizer() magic on this to call libxl_cpuid_dispose()? >> >> We might also want to add something like a .Dispose() method to have >> predictable memory effects. But then do we want to have a .Dispose() >> method on all types that might contain a CpuidPolicyList? Technically >> we're supposed to, so we might have to. (And now I'm having deja vu, >> like we've had this discussion before, but I can't seem to find it.) > > As I've expressed before, I don't think its a good idea to look to the > runtime to fix this sort of problem, so I'd be more inclined to look > into a Dispose like option. But then it does seem weird from an API > perspective to only define Dispose on some types since it introduces a > closer, but incomplete, semantic relationship between libxl and the Go > package. > > WRT the definition of CpuidPolicyList, is the best we can do? Or is > there a way we can hide the use of the C type better so that someone > using this package doesn't need to worry about calling Dispose or > otherwise? I think [1] is where we originally discussed this. If we do have to keep the C pointer around for some reason, I think using SetFinalizer is a necessary backstop to keep the library from leaking. It's all well and good to say, "Make sure you call Dispose()", but I think for a GC'd language that's just going to be too easy to forget; and it will be a huge pain for long-running processes. It is pretty annoying; and this is really the *only* type that has this "opaque structure behind a pointer" property. If we didn't have this type as a type, we'd have to avoid somehow exposing the user to the functions which take and use it. The main place it's used ATM is in DomainBuildInfo. We could explore whether it would be practical to "implement" CpuidPolicyList as a string, and then have toC() call libxl_cpuid_parse_config(). Obviously that means fromC() would fail; but I'm not sure DomainBuildInfo is really a structure passed "out" of libxl anyway. -George
> If we do have to keep the C pointer around for some reason, I think > using SetFinalizer is a necessary backstop to keep the library from > leaking. It's all well and good to say, "Make sure you call Dispose()", > but I think for a GC'd language that's just going to be too easy to > forget; and it will be a huge pain for long-running processes. I understand your motivation for wanting to make this fool-proof, but there are plenty of common examples in Go where it's well-understood that if I call `NewFoo` then I need to `foo.Close()` (defer'd or otherwise). I don't think that alone is a good enough argument for turning to SetFinalizer. But, I'm certainly not advocating for the Dispose option either - as I said I think that would be unfortunate from an API perspective. > If we didn't have this type as a type, we'd have to avoid somehow > exposing the user to the functions which take and use it. The main > place it's used ATM is in DomainBuildInfo. We could explore whether it > would be practical to "implement" CpuidPolicyList as a string, and then > have toC() call libxl_cpuid_parse_config(). Obviously that means > fromC() would fail; but I'm not sure DomainBuildInfo is really a > structure passed "out" of libxl anyway. It's sounding more and more like we need a way to give types an "exported/unexported" attribute in the IDL. Why exactly would fromC be doomed to fail? Just because there is no `libxl_cpuid_to_string` or otherwise? In any case, I think defining it as a string may be a good intermediate option for now (even if it means fromC has to be a no-op). That way we can ensure calls to `libxl_cpuid_dipose` as usual. -NR
On 11/15/19 3:26 PM, Nick Rosbrook wrote: >> If we do have to keep the C pointer around for some reason, I think >> using SetFinalizer is a necessary backstop to keep the library from >> leaking. It's all well and good to say, "Make sure you call Dispose()", >> but I think for a GC'd language that's just going to be too easy to >> forget; and it will be a huge pain for long-running processes. > > I understand your motivation for wanting to make this fool-proof, but > there are plenty of common examples in Go where it's well-understood > that if I call `NewFoo` then I need to `foo.Close()` (defer'd or > otherwise). I don't think that alone is a good enough argument for > turning to SetFinalizer. But, I'm certainly not advocating for the > Dispose option either - as I said I think that would be unfortunate > from an API perspective. > >> If we didn't have this type as a type, we'd have to avoid somehow >> exposing the user to the functions which take and use it. The main >> place it's used ATM is in DomainBuildInfo. We could explore whether it >> would be practical to "implement" CpuidPolicyList as a string, and then >> have toC() call libxl_cpuid_parse_config(). Obviously that means >> fromC() would fail; but I'm not sure DomainBuildInfo is really a >> structure passed "out" of libxl anyway. > > It's sounding more and more like we need a way to give types an > "exported/unexported" attribute in the IDL. > > Why exactly would fromC be doomed to fail? Just because there is no > `libxl_cpuid_to_string` or otherwise? Sorry, I was typing this in a bit of a rush at the end of the day yesterday. :-) Yes, that's what I meant: There's basically no way to read a policy from libxl and then pass it back to libxl (since there's no way to convert libxl_cpuid_policy_list => CpuidPolicyList => libxl_cpuid_policy_list again). But at the moment, a string is the "preferred form of modification" as it were; so if such a read-modify-write feature were implemented, libxl would need to add libxl_cpuid_to_string anyway. (Or give some other modifiable form.) > In any case, I think defining it > as a string may be a good intermediate option for now (even if it > means fromC has to be a no-op). That way we can ensure calls to > `libxl_cpuid_dipose` as usual. Yes, let's do that. -George
> Yes, let's do that.
Okay, will do.
As a point of clarification, should I be waiting until you've reviewed
all patches in v1 before I send v2 of this series? Or do you prefer
that I send a v2 that addresses your review so far?
Thanks,
-NR
On 11/15/19 3:51 PM, Nick Rosbrook wrote: >> Yes, let's do that. > > Okay, will do. > > As a point of clarification, should I be waiting until you've reviewed > all patches in v1 before I send v2 of this series? Or do you prefer > that I send a v2 that addresses your review so far? On the whole I think sending v2 earlier is better, since I'll have the discussions more recently in my head, and so will (hopefully) be able to get an Ack or R-b more quickly. When the development window is open, stuff can be checked in as it's reviewed, making the whole thing easier. To be clear, this is for times when the review of the whole series is taking longer than a few days. If I review 3 patches of a 6-patch series one day, probably better to give me a chance to finish the next day before sending vN+1. :-) But if I stall for a few days, go ahead and resend. Thanks, -George
> On the whole I think sending v2 earlier is better, since I'll have the > discussions more recently in my head, and so will (hopefully) be able to > get an Ack or R-b more quickly. > > When the development window is open, stuff can be checked in as it's > reviewed, making the whole thing easier. > > To be clear, this is for times when the review of the whole series is > taking longer than a few days. If I review 3 patches of a 6-patch > series one day, probably better to give me a chance to finish the next > day before sending vN+1. :-) But if I stall for a few days, go ahead > and resend. Okay thanks for the clarification. I'll plan on sending v2 once I make these changes to CpuidPolicyList. -NR
diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go index d41de253f3..9c384485e1 100644 --- a/tools/golang/xenlight/xenlight.go +++ b/tools/golang/xenlight/xenlight.go @@ -249,6 +249,26 @@ type EvLink struct{} func (el *EvLink) fromC(cel *C.libxl_ev_link) error { return nil } func (el *EvLink) toC() (cel C.libxl_ev_link, err error) { return } +// CpuidPolicyList represents a libxl_cpuid_policy_list. +type CpuidPolicyList struct { + val *C.libxl_cpuid_policy_list +} + +func (cpl *CpuidPolicyList) fromC(ccpl *C.libxl_cpuid_policy_list) error { + cpl.val = ccpl + return nil +} + +func (cpl *CpuidPolicyList) toC() (C.libxl_cpuid_policy_list, error) { + if cpl.val == nil { + var c C.libxl_cpuid_policy_list + return c, nil + } + ccpl := (*C.libxl_cpuid_policy_list)(unsafe.Pointer(cpl.val)) + + return *ccpl, nil +} + type Context struct { ctx *C.libxl_ctx logger *C.xentoollog_logger_stdiostream