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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> 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?

 -George