[11/24] golang/xenlight: define CpuidPolicyList builtin type
diff mbox series

Message ID 5773984ae9308500183adde21cf25837bba39f7f.1570456846.git.rosbrookn@ainfosec.com
State Superseded
Headers show
Series
  • generated Go libxl bindings using IDL
Related show

Commit Message

Nick Rosbrook Oct. 7, 2019, 3:12 p.m. UTC
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(+)

Comments

George Dunlap Nov. 13, 2019, 5:34 p.m. UTC | #1
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
Nick Rosbrook Nov. 14, 2019, 2:58 p.m. UTC | #2
> 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
George Dunlap Nov. 14, 2019, 5:44 p.m. UTC | #3
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
Nick Rosbrook Nov. 15, 2019, 3:26 p.m. UTC | #4
> 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
George Dunlap Nov. 15, 2019, 3:42 p.m. UTC | #5
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
Nick Rosbrook Nov. 15, 2019, 3:51 p.m. UTC | #6
> 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
George Dunlap Nov. 15, 2019, 3:58 p.m. UTC | #7
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
Nick Rosbrook Nov. 15, 2019, 4:06 p.m. UTC | #8
> 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

Patch
diff mbox series

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