On 1/17/20 4:52 PM, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v3 7/8] golang/xenlight: Notify xenlight of
> SIGCHLD"):
>> libxl forks external processes and waits for them to complete; it
>> therefore needs to be notified when children exit.
>>
>> In absence of instructions to the contrary, libxl sets up its own
>> SIGCHLD handlers.
>>
>> Golang always unmasks and handles SIGCHLD itself. libxl thankfully
>> notices this and throws an assert() rather than clobbering SIGCHLD
>> handlers.
>>
>> Tell libxl that we'll be responsible for getting SIGCHLD notifications
>> to it. Arrange for a channel in the context to receive notifications
>> on SIGCHLD, and set up a goroutine that will pass these on to libxl.
>>
>> NB that every libxl context needs a notification; so multiple contexts
>> will each spin up their own goroutine when opening a context, and shut
>> it down on close.
>>
>> libxl also wants to hold on to a const pointer to
>> xenlight_childproc_hooks rather than do a copy; so make a global
>> structure in C space and initialize it once on library creation.
>>
>> While here, add a few comments to make the context set-up a bit easier
>> to follow.
>
> For what it's worth,
>
> Reviewed-by: Ian Jackson <ian.jack...@eu.citrix.com>
>
> However, I don't think I understand golang (and particularly the
> threading model etc.) well enough for that to mean I'm confident that
> this correct.
Thanks -- I mainly just wanted to give you the opportunity to spot any
obvious things I was doing wrong wrt libxl. (For instance, an earlier
version of this patch had me destroying the libxl context before
shutting down the sigchld helper, which is obviously wrong.)
>> +func init() {
>> + // libxl for some reason wants to:
>> + // 1. Retain a copy to this pointer as long as the context is open, and
>> + // 2. Not free it when it's done
>
> I found this comment a bit rude. This is not an unusual approach for
> a pointer in a C API.>
> In Rust this would be called an `immutable borrow': the ctx borrows
> the contents of the pointer, promises not to modify it (and the caller
> ought to promise not to modify it either); but the caller retains
> ownership so when the ctx is done the borrow ends.
I'm sorry to be rude; I've deleted the comment. But none of what you
said is obvious from the documentation; on the contrary:
---
...is equivalent to &{ libxl_sigchld_owner_libxl, 0, 0 }
---
...seems to imply that you can pass it a pointer to the stack. And,
from an interface perspective, that seems obviously better to me --
rather than make the caller promise not to change the contents (to
who-knows-what result if they forget), it's much easier to just take a
local copy and update it with locks next time the function is called.
I was slightly more annoyed because Go's rule about C functions not
retaining pointers to Go memory meant I had to do some un-Golike things
to make this work; but that's nothing to do with libxl.
> Normally in C the struct would be `static const', so there is no need
> to allocate it or free it.
>
> I think that ...
>
>> + // Rather than alloc and free multiple copies, just keep a single
>> + // static copy in the C space (since C code isn't allowed to retain
>> pointers
>> + // to go code), and initialize it once.
>> + C.xenlight_childproc_hooks.chldowner = C.libxl_sigchld_owner_mainloop
>
> ... this is what this is doing ?
In fact, there's a global C variable declared here:
---
#include <libxl.h>
+
+libxl_childproc_hooks xenlight_childproc_hooks;
*/
import "C"
---
...and the line above just initialized it. But on reflection I've
decided to do this:
---
/*
#cgo LDFLAGS: -lxenlight -lyajl -lxentoollog
#include <stdlib.h>
#include <libxl.h>
static const libxl_childproc_hooks childproc_hooks = { .chldowner =
libxl_sigchld_owner_mainloop };
void xenlight_set_chldproc(libxl_ctx *ctx) {
libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
}
*/
import "C"
---
That declares childproc_hooks as static const in the C space; and then
defines a C function which takes a libxl_ctx* and calls
libxl_childproc_setmode appropriately. That way childproc_hooks can
enjoy the safety of static const.
>> +// The alternate would be to register a fork callback, such that the
>> +// xenlight package can make a single list of all children, and only
>> +// notify the specific libxl context(s) that have children woken. But
>> +// it's not clear to me this will be much more work than having the
>> +// xenlight go library do the same thing; doing it in separate go
>> +// threads has the potential to do it in parallel. Leave that as an
>> +// optimization for later if it turns out to be a bottleneck.
>
> I think this is fine.
Thanks.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
