Re: [Xen-devel] [PATCH v3 7/8] golang/xenlight: Notify xenlight of SIGCHLD

From: George Dunlap <george.dunlap@citrix.com>
To: Ian Jackson <ian.jackson@citrix.com>
Cc: Nick Rosbrook <rosbrookn@ainfosec.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v3 7/8] golang/xenlight: Notify xenlight of SIGCHLD
Date: Fri, 17 Jan 2020 17:33:22 +0000	[thread overview]
Message-ID: <61695d47-d419-a2cc-6503-9202e85da6a0@citrix.com> (raw)
In-Reply-To: <24097.58986.657686.844914@mariner.uk.xensource.com>

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.jackson@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