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

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

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@xxxxxxxxxx> wrote:
> > 
> > 
> > 
> >> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@xxxxxxxxx> 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@xxxxxxxxxxxx>
> >> ---
> >> 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