Re: [RESEND PATCH 12/12] golang/xenlight: add NotifyDomainDeath method to Context

[Nick Rosbrook <rosbrookn@gmail.com>]

On Fri, Jun 18, 2021 at 07:31:46PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 18, 2021, at 7:28 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> > 
> > 
> > 
> >> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >> 
> >> Add a helper function to wait for domain death events, and then write
> >> the events to a provided channel. This handles the enabling/disabling of
> >> the event type, freeing the event, and converting it to a Go type. The
> >> caller can then handle the event however they need to. This function
> >> will run until a provided context.Context is cancelled.
> >> 
> >> NotifyDomainDeath spawns two goroutines that return when the
> >> context.Context is done. The first will make sure that the domain death
> >> event is disabled, and that the corresponding event queue is cleared.
> >> The second calls libxl_event_wait, and writes the event to the provided
> >> channel.
> >> 
> >> With this, callers should be able to manage a full domain life cycle.
> >> Add to the comment of DomainCreateNew so that package uses know they
> >> should use this method in conjunction with DomainCreateNew.
> >> 
> >> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> >> ---
> >> tools/golang/xenlight/xenlight.go | 83 ++++++++++++++++++++++++++++++-
> >> 1 file changed, 82 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> >> index 6fb22665cc..8406883433 100644
> >> --- a/tools/golang/xenlight/xenlight.go
> >> +++ b/tools/golang/xenlight/xenlight.go
> >> @@ -53,6 +53,7 @@ import "C"
> >> */
> >> 
> >> import (
> >> +	"context"
> >> 	"fmt"
> >> 	"os"
> >> 	"os/signal"
> >> @@ -1340,7 +1341,9 @@ func (ctx *Context) DeviceUsbdevRemove(domid Domid, usbdev *DeviceUsbdev) error
> >> 	return nil
> >> }
> >> 
> >> -// DomainCreateNew creates a new domain.
> >> +// DomainCreateNew creates a new domain. Callers of DomainCreateNew are
> >> +// responsible for handling the death of the resulting domain. This should be
> >> +// done using NotifyDomainDeath.
> >> func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> >> 	var cdomid C.uint32_t
> >> 	var cconfig C.libxl_domain_config
> >> @@ -1358,6 +1361,84 @@ func (ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> >> 	return Domid(cdomid), nil
> >> }
> >> 
> >> +// NotifyDomainDeath registers an event handler for domain death events for a
> >> +// given domnid, and writes events received to ec. NotifyDomainDeath returns an
> >> +// error if it cannot register the event handler, but other errors encountered
> >> +// are just logged. The goroutine spawned by calling NotifyDomainDeath runs
> >> +// until the provided context.Context's Done channel is closed.
> >> +func (ctx *Context) NotifyDomainDeath(c context.Context, domid Domid, ec chan<- Event) error {
> >> +	var deathw *C.libxl_evgen_domain_death
> >> +
> >> +	ret := C.libxl_evenable_domain_death(ctx.ctx, C.uint32_t(domid), 0, &deathw)
> >> +	if ret != 0 {
> >> +		return Error(ret)
> >> +	}
> >> +
> >> +	// Spawn a goroutine that is responsible for cleaning up when the
> >> +	// passed context.Context's Done channel is closed.
> >> +	go func() {
> >> +		<-c.Done()
> >> +
> >> +		ctx.logd("cleaning up domain death event handler for domain %d", domid)
> >> +
> >> +		// Disable the event generation.
> >> +		C.libxl_evdisable_domain_death(ctx.ctx, deathw)
> >> +
> >> +		// Make sure any events that were generated get cleaned up so they
> >> +		// do not linger in the libxl event queue.
> >> +		var evc *C.libxl_event
> >> +		for {
> >> +			ret := C.libxl_event_check(ctx.ctx, &evc, C.LIBXL_EVENTMASK_ALL, nil, nil)
> >> +			if ret != 0 {
> >> +				return
> >> +			}
> >> +			C.libxl_event_free(ctx.ctx, evc)
> > 
> > I have to admit, I don’t really understand how the libxl event stuff is supposed to work.  But it looks like this will drain all events of any type, for any domain, associated with this context?
> > 
> > So if you had two domains, and called NotifyDomainDeath() on both with different contexts, and you closed the one context, you might miss events from the other context?
> > 
> > Or, suppose you did this:
> > * ctx.NotifyDomainDeath(ctx1, dom1, ec1)
> > * ctx.NotifyDiskEject(ctx2, dom1, ec2)
> > * ctx1CancelFunc()
> > 
> > Wouldn’t there be a risk that the disk eject message would get lost?
> > 
> > Ian, any suggestions for the right way to use these functions in this scenario?
> 
> It looks like one option would be to add a “predicate” function filter, to filter by type and domid.
> 
> It looks like the other option would be to try to use libxl_event_register_callbacks().  We could have the C callback pass all the events to a goroutine which would act as a dispatcher.
> 
After a brief look at the documentation within libxl_event.h, it seems
using predicate functions would be the easiest solution given the
current layout. Though I will look closer at using
libxl_event_register_callbacks before sending a v2.

Thanks,
NR