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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>

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.

> +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.

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 ?

> +// This should "play nicely" with other users of SIGCHLD as long as
> +// they don't reap libxl's processes.

I assume that nothing in golang will do this.  If something calls a
non-process-specific variant of wait* then you would need to somehow
capture the results and feed them to libxl_childproc_exited.

> +// 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,
Ian.

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