[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC] Generating Go bindings for libxl


  • To: Nicholas Rosbrook <rosbrookn@xxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Thu, 12 Sep 2019 15:37:52 +0100
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=george.dunlap@xxxxxxxxxx; spf=Pass smtp.mailfrom=George.Dunlap@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABtCRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT6JAlcEEwEKAEECGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEWIQTXqBy2bTNXPzpOYFimNjwxBZC0bQUCXEowWQUJDCJ7dgAKCRCmNjwx BZC0beKvEACJ75YlJXd7TnNHgFyiCJkm/qPeoQ3sFGSDZuZh7SKcdt9+3V2bFEb0Mii1hQaz 3hRqZb8sYPHJrGP0ljK09k3wf8k3OuNxziLQBJyzvn7WNlE4wBEcy/Ejo9TVBdA4ph5D0YaZ nqdsPmxe/xlTFuSkgu4ep1v9dfVP1TQR0e+JIBa/Ss+cKC5intKm+8JxpOploAHuzaPu0L/X FapzsIXqgT9eIQeBEgO2hge6h9Jov3WeED/vh8kA7f8c6zQ/gs5E7VGALwsiLrhr0LZFcKcw kI3oCCrB/C/wyPZv789Ra8EXbeRSJmTjcnBwHRPjnjwQmetRDD1t+VyrkC6uujT5jmgOBzaj KCqZ8PcMAssOzdzQtKmjUQ2b3ICPs2X13xZ5M5/OVs1W3TG5gkvMh4YoHi4ilFnOk+v3/j7q 65FG6N0JLb94Ndi80HkIOQQ1XVGTyu6bUPaBg3rWK91Csp1682kD/dNVF3FKHrRLmSVtmEQR 5rK0+VGc/FmR6vd4haKGWIRuPxzg+pBR77avIZpU7C7+UXGuZ5CbHwIdY8LojJg2TuUdqaVj yxmEZLOA8rVHipCGrslRNthVbJrGN/pqtKjCClFZHIAYJQ9EGLHXLG9Pj76opfjHij3MpR3o pCGAh6KsCrfrsvjnpDwqSbngGyEVH030irSk4SwIqZ7FwLkBDQRUWmc6AQgAzpc8Ng5Opbrh iZrn69Xr3js28p+b4a+0BOvC48NfrNovZw4eFeKIzmI/t6EkJkSqBIxobWRpBkwGweENsqnd 0qigmsDw4N7J9Xx0h9ARDqiWxX4jr7u9xauI+CRJ1rBNO3VV30QdACwQ4LqhR/WA+IjdhyMH wj3EJGE61NdP/h0zfaLYAbvEg47/TPThFsm4m8Rd6bX7RkrrOgBbL/AOnYOMEivyfZZKX1vv iEemAvLfdk2lZt7Vm6X/fbKbV8tPUuZELzNedJvTTBS3/l1FVz9OUcLDeWhGEdlxqXH0sYWh E9+PXTAfz5JxKH+LMetwEM8DbuOoDIpmIGZKrZ+2fQARAQABiQNbBBgBCgAmAhsCFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKMJ4FCQnQ/OQBKcBdIAQZAQoABgUCVFpnOgAKCRCyFcen x4Qb7cXrCAC0qQeEWmLa9oEAPa+5U6wvG1t/mi22gZN6uzQXH1faIOoDehr7PPESE6tuR/vI CTTnaSrd4UDPNeqOqVF07YexWD1LDcQG6PnRqC5DIX1RGE3BaSaMl2pFJP8y+chews11yP8G DBbxaIsTcHZI1iVIC9XLhoeegWi84vYc8F4ziADVfowbmbvcVw11gE8tmALCwTeBeZVteXjh 0OELHwrc1/4j4yvENjIXRO+QLIgk43kB57Upr4tP2MEcs0odgPM+Q+oETOJ00xzLgkTnLPim C1FIW2bOZdTj+Uq6ezRS2LKsNmW+PRRvNyA5ojEbA/faxmAjMZtLdSSSeFK8y4SoCRCmNjwx BZC0bevWEACRu+GyQgrdGmorUptniIeO1jQlpTiP5WpVnk9Oe8SiLoXUhXXNj6EtzyLGpYmf kEAbki+S6WAKnzZd3shL58AuMyDxtFNNjNeKJOcl6FL7JPBIIgIp3wR401Ep+/s5pl3Nw8Ii 157f0T7o8CPb54w6S1WsMkU78WzTxIs/1lLblSMcvyz1Jq64g4OqiWI85JfkzPLlloVf1rzy ebIBLrrmjhCE2tL1RONpE/KRVb+Q+PIs5+YcZ+Q1e0vXWA7NhTWFbWx3+N6WW6gaGpbFbopo FkYRpj+2TA5cX5zW148/xU5/ATEb5vdUkFLUFVy5YNUSyeBHuaf6fGmBrDc47rQjAOt1rmyD 56MUBHpLUbvA6NkPezb7T6bQpupyzGRkMUmSwHiLyQNJQhVe+9NiJJvtEE3jol0JVJoQ9WVn FAzPNCgHQyvbsIF3gYkCYKI0w8EhEoH5FHYLoKS6Jg880IY5rXzoAEfPvLXegy6mhYl+mNVN QUBD4h9XtOvcdzR559lZuC0Ksy7Xqw3BMolmKsRO3gWKhXSna3zKl4UuheyZtubVWoNWP/bn vbyiYnLwuiKDfNAinEWERC8nPKlv3PkZw5d3t46F1Dx0TMf16NmP+azsRpnMZyzpY8BL2eur feSGAOB9qjZNyzbo5nEKHldKWCKE7Ye0EPEjECS1gjKDwbkBDQRUWrq9AQgA7aJ0i1pQSmUR 6ZXZD2YEDxia2ByR0uZoTS7N0NYv1OjU8v6p017u0Fco5+Qoju/fZ97ScHhp5xGVAk5kxZBF DT4ovJd0nIeSr3bbWwfNzGx1waztfdzXt6n3MBKr7AhioB1m+vuk31redUdnhbtvN7O40MC+ fgSk5/+jRGxY3IOVPooQKzUO7M51GoOg4wl9ia3H2EzOoGhN2vpTbT8qCcL92ZZZwkBRldoA Wn7c1hEKSTuT3f1VpSmhjnX0J4uvKZ1V2R7rooKJYFBcySC0wa8aTmAtAvLgfcpe+legOtgq DKzLuN45xzEjyjCiI521t8zxNMPJY9FiCPNv0sCkDwARAQABiQI8BBgBCgAmAhsMFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKNJYFCQnQrVkACgkQpjY8MQWQtG2Xxg//RrRP+PFYuNXt 9C5hec/JoY24TkGPPd2tMC9usWZVImIk7VlHlAeqHeE0lWU0LRGIvOBITbS9izw6fOVQBvCA Fni56S12fKLusWgWhgu03toT9ZGxZ9W22yfw5uThSHQ4y09wRWAIYvhJsKnPGGC2KDxFvtz5 4pYYNe8Icy4bwsxcgbaSFaRh+mYtts6wE9VzyJvyfTqbe8VrvE+3InG5rrlNn51AO6M4Wv20 iFEgYanJXfhicl0WCQrHyTLfdB5p1w+072CL8uryHQVfD0FcDe+J/wl3bmYze+aD1SlPzFoI MaSIXKejC6oh6DAT4rvU8kMAbX90T834Mvbc3jplaWorNJEwjAH/r+v877AI9Vsmptis+rni JwUissjRbcdlkKBisoUZRPmxQeUifxUpqgulZcYwbEC/a49+WvbaYUriaDLHzg9xisijHwD2 yWV8igBeg+cmwnk0mPz8tIVvwi4lICAgXob7HZiaqKnwaDXs4LiS4vdG5s/ElnE3rIc87yru 24n3ypeDZ6f5LkdqL1UNp5/0Aqbr3EiN7/ina4YVyscy9754l944kyHnnMRLVykg0v+kakj0 h0RJ5LbfLAMM8M52KIA3y14g0Fb7kHLcOUMVcgfQ3PrN6chtC+5l6ouDIlSLR3toxH8Aam7E rIFfe2Dk+lD9A9BVd2rfoHA=
  • Cc: "anthony.perard@xxxxxxxxxx" <anthony.perard@xxxxxxxxxx>, Brendan Kerrigan <kerriganb@xxxxxxxxxxxx>, "ian.jackson@xxxxxxxxxxxxx" <ian.jackson@xxxxxxxxxxxxx>, Nicolas Belouin <nicolas.belouin@xxxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>
  • Delivery-date: Thu, 12 Sep 2019 14:38:03 +0000
  • Ironport-sdr: dcQuIcIE0LEsPH/38nbDv3mU5dqQfnABHFr3s2G2+rB/5fSncHN3T1GyMMjkP5mMYCUnLpjDAY z9Yah1Xy8CVN/r8GKf4sMT3rJWxGDl6mba2KZJpqcZnjOtsb2BEmRFDe9yquFpHLWwc5jQGWnm R4Bkvpx/OFB/PEuSTUgDdKVknTTCGNBh9VcD3/PGtrDvekwy4vU+ERJnNZKxQe5i7w0DWazNnD a5EZW5ImAJrFrB1JCo7sljmmbK4es/+0s0LXJ0lgpdTiWSToiKwPJnzMislpTygKYrblGZu677 fSA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 9/11/19 9:25 PM, Nicholas Rosbrook wrote:
> Hi George,
> 
> I made some more progress on gengotypes.py [1].
[snip]
> What are your thoughts on these implementations so far?

Great!  Overall it looks like it's really making progress, which is
exciting.

> First, I implemented the KeyedUnion translation that we talked about. 
> You can see an example of the generated code in [2].

That looks about like we expected -- tolerable and functional, to be
certain, but LotsOfReallyLongTypeNames.

I was chatting with Anthony today, and thought I'd just toss the idea
out there for discussion.

The only purpose of unions in these structures is to save space (as
opposed to other kinds of unions are specifically designed to allow
different "views" of the same underlying data).  We're replacing the
unions with structures which will be 1) allocated separately, and 2)
require casting and type assertions to handle properly.  This will save
*some* space, but at the cost of a certain amount of complexity, and
run-time overhead.

What we just defined three separate elements in the struct?  E.g.:

type DomainInfo struct {
        /* etc */
        Type struct {
                Key int
                Hvm struct {
                        Firmware string
                        /* ... */
                }
                Pv struct {
                        /* ... */
                }
                Pvh struct {
                        /* ... */
                }
        }
}

This obviously means keeping a whole load of useless HVM and PV fields
around when you just want to run PVH, but it you can simply do this:

if ( di.Type.Key == libxl.DomainTypeHvm ) {
   /* ... */
   firmware := di.Type.Hvm.Firmware;
}

This also mean you could make a mistake and access the HVM fields for a
PV domain, and you'd get neither a compile-time nor a run-time error.

Anyway, like I said, just tossing it out there.  If we decide we don't
want duplicate structs, I think your implementation looks about as good
as it can be.

> Second, I took a first pass at the C-to-Go type marshaling. I defined a
> "marshaler" interface in [3], which allows the convenience function
> `func fromC(m marshaler, ctype interface{}) (err error)`. My primary 
> motivation for this interface is to allow the generated code to call panic
> rather than checking for and handling errors. However, the previously
> mentioned convenience function will recover from those panics and return
> the appropriate error. So, each generated struct implements this interface.
> See the generated code in [4].

So the advantage of this is that you can just call:

    fromC(&di, &cdi)

Rather than:

    di.fromC(&cdi)

?

But the cost for this is that we're switching from static type-checking
to dynamic type-checking.  If in the first example, cdi is the wrong
type (for instance, if we forget the & at the front), everything will
compile, and we won't notice unless the function actually gets called.
In the second example, if we're not trying to implement a generic
"marshaler" method, we can define the function signature to specify
exactly what pointer we need.

I don't want to say I'd rule it out, but it doesn't seem to me like the
convenience is worth the cost (unless there's another advantage I'm
missing).

> You'll also notice in [4] that I defined C structs in the cgo preamble which
> correspond to the Go KeyedUnion structs, e.g. DomainBuildInfoTypeUnionPv.
> Since cgo treats C unions a byte slice, we need to do an unsafe.Pointer 
> conversion
> to some struct to be able to access the fields of a union. So, I thought it 
> would
> make the most sense to do the cast to a C type, and then convert those fields
> to Go types accordingly. See [5] for example.

Right -- that looks like just about the only option?  Anyway, it's a
good option; no point spending a lot of time looking for ways to improve
something that's only really going to live inside one generated file.

> I was able to write a couple examples to demonstrate the generated code is 
> working, but I had to make some small changes to the existing code WRT
> libxl builtin types (not committed to my branch). So, I thought we should 
> decide
> how these builtin types will be defined in Go. This is what I was thinking so 
> far:
> 
> Defbool (?)

Well this is really defined by the interface.  libxl.h has this defined as

struct {
   int val;
}


We could basically do the same thing; but make `val` non-exported.

> Domid (already exists)
> Devid => int
> Uuid => [16]byte
> Mac => [6]byte
> Bitmap (already exists)

Yup

> CpuidPolicyList (?)

So this is an interesting one.  libxl__cpuid_policy is essentially a
type containing all non-exported fields.  (i.e., the actual elements are
defined inside libxl_internal.h, and the outside world only gets pointers).

And it's a list terminated by a specific value inside
libxl__cpuid_policy, which means the list itself is basically opaque
entirely.

I'd be tempted to say just do something like:

type CpuidPolicyList struct {
    val C.libxl_cpuid_policy_list
};

A part of me thinks even something like this wouldn't be terrible:

type CpuidPolicyList C.libxl_cpuid_policy_list

It "leaks" the internals out to the callers, but it also means you don't
have to do all this faff of marshalling / unmarshalling what's
essentially just a pointer.

> StringList => [ ]string
> KeyValueList => map[string]string
> Hwcap (already exists, but should be re-factored to be like Bitmap to hide 
> the C type)
> MsVmGenid => [16]byte

Should probably be C.LIBXL_MS_VM_GENID_LEN, but yes.

> EvLink (?)

It sort of looks like this is an entirely internal thing that libxl
uses.  I think to begin with we can just declare this as an empty
struct, and figure out what to put in it once it becomes more clear how
it needs to be used.

Thanks for all your work on this!

Peace,
 -George

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.