Re: [RESEND PATCH 08/12] golang/xenlight: add functional options to configure Context

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Fri, Jun 18, 2021 at 02:44:15PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@xxxxxxxxx> wrote:
> > 
> > Add a ContextOption type to support functional options in NewContext.
> > Then, add a variadic ContextOption parameter to NewContext, which allows
> > callers to specify 0 or more configuration options.
> > 
> > For now, just add the WithLogLevel option so that callers can set the
> > log level of the Context's xentoollog_logger. Future configuration
> > options can be created by adding an appropriate field to the
> > contextOptions struct and creating a With<OptionName> function to return
> > a ContextOption
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> > ---
> > tools/golang/xenlight/xenlight.go | 44 +++++++++++++++++++++++++++++--
> > 1 file changed, 42 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go 
> > b/tools/golang/xenlight/xenlight.go
> > index f68d7b6e97..65f93abe32 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -136,7 +136,7 @@ func sigchldHandler(ctx *Context) {
> > }
> > 
> > // NewContext returns a new Context.
> > -func NewContext() (ctx *Context, err error) {
> > +func NewContext(opts ...ContextOption) (ctx *Context, err error) {
> >     ctx = &Context{}
> > 
> >     defer func() {
> > @@ -146,8 +146,19 @@ func NewContext() (ctx *Context, err error) {
> >             }
> >     }()
> > 
> > +   // Set the default context options. These fields may
> > +   // be modified by the provided opts.
> > +   copts := &contextOptions{
> > +           logLevel: LogLevelError,
> > +   }
> > +
> > +   for _, opt := range opts {
> > +           opt.apply(copts)
> > +   }
> > +
> >     // Create a logger
> > -   ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
> > +   ctx.logger = C.xtl_createlogger_stdiostream(C.stderr,
> > +           C.xentoollog_level(copts.logLevel), 0)
> > 
> >     // Allocate a context
> >     ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0,
> > @@ -201,6 +212,35 @@ func (ctx *Context) Close() error {
> >     return nil
> > }
> > 
> > +type contextOptions struct {
> > +   logLevel LogLevel
> > +}
> > +
> > +// ContextOption is used to configure options for a Context.
> > +type ContextOption interface {
> > +   apply(*contextOptions)
> > +}
> > +
> > +type funcContextOption struct {
> > +   f func(*contextOptions)
> > +}
> > +
> > +func (fco *funcContextOption) apply(c *contextOptions) {
> > +   fco.f(c)
> > +}
> 
> Why all this convolution with interfaces and such, rather than just defining 
> ContextOption as a function pointer?  Is it just to keep contextOptions out 
> of the documentation page?

Part of the motivation for using functional options is to abstract the
"options" struct, yes. This allows internal defaults to be applied more
easily -- if you require e.g. a ContextOptions struct to be passed by
the caller, how do you know if they intended to override a default, or
if they just didn't set the field? Additionally, using the ContextOption
as an interface allows variadic arguments, which are just convenient for
API users -- the same NewContext function can be used whether you need
to pass 3 options or 0.

The reason we use ContextOption as an interface, rather than function
pointer of sorts is for flexibility in the signatures of ContextOption
implementations. E.g., we could have

func WithLogLevel(lvl LogLevel) ContextOption
func WithLogContext(s string) ContextOption
func WithFooAndBar(s string, n int) ContextOption

See [1] for more background on this pattern.

Thanks,
NR

[1] https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis