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

[George Dunlap <george.dunlap@citrix.com>]

On 1/17/20 6:13 PM, Nick Rosbrook wrote:
>>  // Context represents a libxl_ctx.
>>  type Context struct {
>> -       ctx    *C.libxl_ctx
>> -       logger *C.xentoollog_logger_stdiostream
>> +       ctx         *C.libxl_ctx
>> +       logger      *C.xentoollog_logger_stdiostream
>> +       sigchld     chan os.Signal
>> +       sigchldDone chan bool
> 
> It's preferred to use `chan struct{}` for this pattern; it makes it
> clear that the data sent over the channel has no meaning, and is only
> intended to be used for synchronization.

OK.  I think it looks ugly, but there's certainly a signalling value to
having it really be empty.

> 
>> +       // ...and arrange to keep that promise.
>> +       ctx.sigchld = make(chan os.Signal, 2)
>> +       ctx.sigchldDone = make(chan bool, 1)
>> +       signal.Notify(ctx.sigchld, syscall.SIGCHLD)
>> +
>> +       go sigchldHandler(ctx)
> 
> It could be useful to add a comment here that explains the lifetime of
> this goroutine, i.e. it returns when the context is Close()'d.

Ack.

>>  // Close closes the Context.
>>  func (ctx *Context) Close() error {
>> +       // Tell our SIGCHLD notifier to shut down, and wait for it to exit
>> +       // before we free the context.
>> +       if ctx.sigchld == nil {
> 
> Shouldn't this be `if ctx.sigchld != nil`?

Er, yes, indeed it should.  This has gone through too many iterations...

 -George

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