From f2b1a8a9ff836cec6261f0a1dfee9da35b1e36db Mon Sep 17 00:00:00 2001 From: Dominic Della Valle Date: Mon, 10 Jul 2023 13:33:25 -0400 Subject: [PATCH] commands: client/server - refactor maddr+log flags The `-api-server` flag no longer uses a magic type to distinguish default values. Instead, client+server code will check if the flag was left unset, and provides a default value if so. The `-verbose` flag is no longer shared through embedding. It is still present in client and server commands, but the implementations initialize optional values (like `log` interfaces) at parse time rather at execution time. --- internal/commands/client.go | 179 ++++++++++++++++++---------------- internal/commands/daemon.go | 77 ++++++++------- internal/commands/flag.go | 23 ----- internal/commands/mount.go | 3 - internal/commands/proc.go | 6 +- internal/commands/shared.go | 10 -- internal/commands/shutdown.go | 2 +- internal/commands/unmount.go | 6 +- 8 files changed, 141 insertions(+), 165 deletions(-) diff --git a/internal/commands/client.go b/internal/commands/client.go index 3a48855d..18f64c62 100644 --- a/internal/commands/client.go +++ b/internal/commands/client.go @@ -17,59 +17,74 @@ import ( "github.com/djdv/p9/p9" "github.com/multiformats/go-multiaddr" manet "github.com/multiformats/go-multiaddr/net" + "github.com/u-root/uio/ulog" ) type ( Client p9.Client clientSettings struct { serviceMaddr multiaddr.Multiaddr + log ulog.Logger exitInterval time.Duration - sharedSettings } clientOption func(*clientSettings) error clientOptions []clientOption - // defaultClientMaddr distinguishes - // the default maddr value, from an arbitrary maddr value. - // I.e. even if the underlying multiaddrs are the same - // only the flag's default value should be of this type. - // Implying the flag was not provided/set explicitly. - defaultClientMaddr struct{ multiaddr.Multiaddr } ) const ( - exitIntervalDefault = 30 * time.Second - errServiceNotFound = generic.ConstError("could not find service instance") + exitIntervalDefault = 30 * time.Second + errServiceConnection = generic.ConstError("could not connect to service") + errCouldNotDial = generic.ConstError("could not dial") ) func (cs *clientSettings) getClient(autoLaunchDaemon bool) (*Client, error) { var ( serviceMaddr = cs.serviceMaddr - clientOpts []p9.ClientOpt + options []p9.ClientOpt ) - if cs.verbose { - // TODO: less fancy prefix and/or out+prefix from CLI flags - clientLog := log.New(os.Stdout, "⬇️ client - ", log.Lshortfile) - clientOpts = append(clientOpts, p9.WithClientLogger(clientLog)) + if log := cs.log; log != nil { + options = append(options, p9.WithClientLogger(log)) } - if autoLaunchDaemon { - if _, wasUnset := serviceMaddr.(defaultClientMaddr); wasUnset { - return connectOrLaunchLocal(cs.exitInterval, clientOpts...) + var serviceMaddrs []multiaddr.Multiaddr + if serviceMaddr != nil { + autoLaunchDaemon = false + serviceMaddrs = []multiaddr.Multiaddr{serviceMaddr} + } else { + var err error + if serviceMaddrs, err = allServiceMaddrs(); err != nil { + return nil, fmt.Errorf( + "%w: %w", + errServiceConnection, err, + ) } } - return Connect(serviceMaddr, clientOpts...) + client, err := connect(serviceMaddrs, options...) + if err == nil { + return client, nil + } + if autoLaunchDaemon && + errors.Is(err, errCouldNotDial) { + return launchAndConnect(cs.exitInterval, options...) + } + return nil, err } func (co *clientOptions) BindFlags(flagSet *flag.FlagSet) { - var sharedOptions sharedOptions - (&sharedOptions).BindFlags(flagSet) - *co = append(*co, func(cs *clientSettings) error { - subset, err := sharedOptions.make() - if err != nil { - return err - } - cs.sharedSettings = subset - return nil - }) + const ( + verboseName = "verbose" + verboseUsage = "enable client message logging" + ) + flagSetFunc(flagSet, verboseName, verboseUsage, co, + func(verbose bool, settings *clientSettings) error { + if verbose { + const ( + prefix = "⬇️ client - " + flags = 0 + ) + settings.log = log.New(os.Stderr, prefix, flags) + } + return nil + }) const ( exitName = exitAfterFlagName exitUsage = "passed to the daemon command if we launch it" + @@ -88,8 +103,19 @@ func (co *clientOptions) BindFlags(flagSet *flag.FlagSet) { settings.serviceMaddr = value return nil }) + serviceMaddrs, err := allServiceMaddrs() + if err != nil { + panic(err) + } + maddrStrings := make([]string, len(serviceMaddrs)) + for i, maddr := range serviceMaddrs { + maddrStrings[i] = "`" + maddr.String() + "`" + } flagSet.Lookup(serverFlagName). - DefValue = defaultServerMaddr().String() + DefValue = fmt.Sprintf( + "one of: %s", + strings.Join(maddrStrings, ", "), + ) } func (co clientOptions) make() (clientSettings, error) { @@ -99,19 +125,9 @@ func (co clientOptions) make() (clientSettings, error) { if err := generic.ApplyOptions(&settings, co...); err != nil { return clientSettings{}, err } - if err := settings.fillDefaults(); err != nil { - return clientSettings{}, err - } return settings, nil } -func (cs *clientSettings) fillDefaults() error { - if cs.serviceMaddr == nil { - cs.serviceMaddr = defaultServerMaddr() - } - return nil -} - func (c *Client) getListeners() ([]multiaddr.Multiaddr, error) { listenersDir, err := (*p9.Client)(c).Attach(listenersFileName) if err != nil { @@ -124,44 +140,26 @@ func (c *Client) getListeners() ([]multiaddr.Multiaddr, error) { return maddrs, listenersDir.Close() } -func connectOrLaunchLocal(exitInterval time.Duration, options ...p9.ClientOpt) (*Client, error) { - conn, err := findLocalServer() - if err == nil { - return newClient(conn, options...) - } - if !errors.Is(err, errServiceNotFound) { - return nil, err - } - return launchAndConnect(exitInterval, options...) -} - func launchAndConnect(exitInterval time.Duration, options ...p9.ClientOpt) (*Client, error) { daemon, ipc, stderr, err := spawnDaemonProc(exitInterval) if err != nil { return nil, err } - var ( - errs []error - killProc = func() error { - return errors.Join( - maybeKill(daemon), - daemon.Process.Release(), - ) - } - ) + killProc := func() error { + return errors.Join( + maybeKill(daemon), + daemon.Process.Release(), + ) + } maddrs, err := getListenersFromProc(ipc, stderr, options...) if err != nil { - errs = append(errs, err) + errs := []error{err} if err := killProc(); err != nil { errs = append(errs, err) } return nil, errors.Join(errs...) } - conn, err := firstDialable(maddrs) - if err != nil { - return nil, errors.Join(err, killProc()) - } - client, err := newClient(conn, options...) + client, err := connect(maddrs) if err != nil { return nil, errors.Join(err, killProc()) } @@ -182,9 +180,16 @@ func launchAndConnect(exitInterval time.Duration, options ...p9.ClientOpt) (*Cli } func Connect(serverMaddr multiaddr.Multiaddr, options ...p9.ClientOpt) (*Client, error) { - conn, err := manet.Dial(serverMaddr) + return connect([]multiaddr.Multiaddr{serverMaddr}, options...) +} + +func connect(maddrs []multiaddr.Multiaddr, options ...p9.ClientOpt) (*Client, error) { + conn, err := firstDialable(maddrs...) if err != nil { - return nil, fmt.Errorf("could not connect to service: %w", err) + return nil, fmt.Errorf( + "%w: %w", + errServiceConnection, err, + ) } return newClient(conn, options...) } @@ -192,29 +197,27 @@ func Connect(serverMaddr multiaddr.Multiaddr, options ...p9.ClientOpt) (*Client, func newClient(conn io.ReadWriteCloser, options ...p9.ClientOpt) (*Client, error) { client, err := p9.NewClient(conn, options...) if err != nil { - return nil, err + return nil, fmt.Errorf( + "could not create client: %w", + err, + ) } return (*Client)(client), nil } -// findLocalServer searches a set of local addresses -// and returns the first dialable maddr it finds. -// [errServiceNotFound] will be returned if none are dialable. -func findLocalServer() (manet.Conn, error) { - allMaddrs, err := allServiceMaddrs() - if err != nil { - return nil, err - } - return firstDialable(allMaddrs) -} - func allServiceMaddrs() ([]multiaddr.Multiaddr, error) { var ( userMaddrs, uErr = userServiceMaddrs() systemMaddrs, sErr = systemServiceMaddrs() serviceMaddrs = append(userMaddrs, systemMaddrs...) ) - return serviceMaddrs, errors.Join(uErr, sErr) + if err := errors.Join(uErr, sErr); err != nil { + return nil, fmt.Errorf( + "could not retrieve service maddrs: %w", + err, + ) + } + return serviceMaddrs, nil } // TODO: [Ame] docs. @@ -231,21 +234,25 @@ func systemServiceMaddrs() ([]multiaddr.Multiaddr, error) { return hostServiceMaddrs() } -func firstDialable(maddrs []multiaddr.Multiaddr) (manet.Conn, error) { +func firstDialable(maddrs ...multiaddr.Multiaddr) (manet.Conn, error) { for _, maddr := range maddrs { if conn, err := manet.Dial(maddr); err == nil { return conn, nil } } + return nil, fmt.Errorf( + "%w any of: %s", + errCouldNotDial, + formatMaddrs(maddrs), + ) +} + +func formatMaddrs(maddrs []multiaddr.Multiaddr) string { maddrStrings := make([]string, len(maddrs)) for i, maddr := range maddrs { maddrStrings[i] = maddr.String() } - var ( - cErr error = errServiceNotFound - fmtString = strings.Join(maddrStrings, ", ") - ) - return nil, fmt.Errorf("%w: tried: %s", cErr, fmtString) + return strings.Join(maddrStrings, ", ") } func servicePathsToServiceMaddrs(servicePaths ...string) ([]multiaddr.Multiaddr, error) { diff --git a/internal/commands/daemon.go b/internal/commands/daemon.go index a7a8d566..9123e66e 100644 --- a/internal/commands/daemon.go +++ b/internal/commands/daemon.go @@ -7,7 +7,7 @@ import ( "fmt" "io" "io/fs" - golog "log" + "log" "net" "os" "reflect" @@ -28,10 +28,10 @@ import ( type ( daemonSettings struct { - serverMaddrs []multiaddr.Multiaddr - exitInterval time.Duration + systemLog, protocolLog ulog.Logger + serverMaddrs []multiaddr.Multiaddr + exitInterval time.Duration nineIDs - sharedSettings permissions fs.FileMode } daemonOption func(*daemonSettings) error @@ -97,16 +97,21 @@ const ( ) func (do *daemonOptions) BindFlags(flagSet *flag.FlagSet) { - var sharedOptions sharedOptions - (&sharedOptions).BindFlags(flagSet) - *do = append(*do, func(ds *daemonSettings) error { - subset, err := sharedOptions.make() - if err != nil { - return err - } - ds.sharedSettings = subset - return nil - }) + const ( + verboseName = "verbose" + verboseUsage = "enable server message logging" + ) + flagSetFunc(flagSet, verboseName, verboseUsage, do, + func(verbose bool, settings *daemonSettings) error { + if verbose { + const ( + prefix = "⬆️ server - " + flags = 0 + ) + settings.systemLog = log.New(os.Stderr, prefix, flags) + } + return nil + }) const serverUsage = "listening socket `maddr`" + "\ncan be specified multiple times and/or comma separated" flagSetFunc(flagSet, serverFlagName, serverUsage, do, @@ -114,8 +119,12 @@ func (do *daemonOptions) BindFlags(flagSet *flag.FlagSet) { settings.serverMaddrs = append(settings.serverMaddrs, value...) return nil }) + userMaddrs, err := userServiceMaddrs() + if err != nil { + panic(err) + } flagSet.Lookup(serverFlagName). - DefValue = defaultServerMaddr().String() + DefValue = userMaddrs[0].String() const ( exitName = exitAfterFlagName exitUsage = "check every `interval` (e.g. \"30s\") and shutdown the daemon if its idle" @@ -178,9 +187,14 @@ func (do daemonOptions) make() (daemonSettings, error) { return daemonSettings{}, err } if settings.serverMaddrs == nil { - settings.serverMaddrs = []multiaddr.Multiaddr{ - defaultServerMaddr(), + userMaddrs, err := userServiceMaddrs() + if err != nil { + return daemonSettings{}, err } + settings.serverMaddrs = userMaddrs[0:1:1] + } + if settings.systemLog == nil { + settings.systemLog = ulog.Null } return settings, nil } @@ -214,7 +228,10 @@ func daemonExecute(ctx context.Context, options ...daemonOption) error { path = fsys.path root = fsys.root log = system.log - server = makeServer(newAttacher(path, root), log) + server = makeServer( + newAttacher(path, root), + settings.protocolLog, + ) stopSend, stopReceive = makeStoppers(ctx) lsnStop, @@ -292,10 +309,13 @@ func makeStoppers(ctx context.Context) (wgShutdown, <-chan shutdownDisposition) } func makeServer(fsys p9.Attacher, log ulog.Logger) *p9net.Server { - server := p9net.NewServer(fsys, - p9net.WithServerLogger(log), - ) - return server + var options []p9net.ServerOpt + if log != nil { + options = []p9net.ServerOpt{ + p9net.WithServerLogger(log), + } + } + return p9net.NewServer(fsys, options...) } func splitStopper(shutdownLevels <-chan shutdownDisposition) (_, _, _ <-chan shutdownDisposition) { @@ -454,23 +474,12 @@ func newSystem(ctx context.Context, set *daemonSettings) (*daemonSystem, error) fsys, err = newFileSystem(ctx, uid, gid) system = &daemonSystem{ files: fsys, - log: newDaemonLog(set.verbose), + log: set.systemLog, } ) return system, err } -func newDaemonLog(verbose bool) ulog.Logger { - if !verbose { - return ulog.Null - } - const ( - prefix = "⬆️ server - " - flags = golog.Lshortfile - ) - return golog.New(os.Stderr, prefix, flags) -} - func newFileSystem(ctx context.Context, uid p9.UID, gid p9.GID) (fileSystem, error) { const permissions = p9fs.ReadUser | p9fs.WriteUser | p9fs.ExecuteUser | p9fs.ReadGroup | p9fs.ExecuteGroup | diff --git a/internal/commands/flag.go b/internal/commands/flag.go index 45b64733..796685c8 100644 --- a/internal/commands/flag.go +++ b/internal/commands/flag.go @@ -33,12 +33,6 @@ type ( ] interface { ~[]OT } - sharedSettings struct { - verbose bool - } - sharedOption func(*sharedSettings) error - sharedOptions []sharedOption - // standard [flag.funcValue] extended // for [command.ValueNamer]. // (Because standard uses internal types @@ -87,23 +81,6 @@ func makeWithOptions[OT generic.OptionFunc[T], T any](options ...OT) (T, error) return settings, generic.ApplyOptions(&settings, options...) } -func (so *sharedOptions) BindFlags(flagSet *flag.FlagSet) { - const ( - verboseName = "verbose" - verboseDefault = false - verboseUsage = "enable log messages" - ) - flagSetFunc(flagSet, verboseName, verboseUsage, so, - func(value bool, settings *sharedSettings) error { - settings.verbose = value - return nil - }) -} - -func (so sharedOptions) make() (sharedSettings, error) { - return makeWithOptions(so...) -} - func parseID[id fuseID | p9.UID | p9.GID](arg string) (id, error) { const nobody = "nobody" if arg == nobody { diff --git a/internal/commands/mount.go b/internal/commands/mount.go index 7e7bebca..1513245e 100644 --- a/internal/commands/mount.go +++ b/internal/commands/mount.go @@ -192,9 +192,6 @@ func (mo mountCmdOptions[HT, GT, HM, GM, HC, GC]) make() (mountCmdSettings[HM, G if err := generic.ApplyOptions(&settings, mo...); err != nil { return settings, err } - if err := settings.clientSettings.fillDefaults(); err != nil { - return settings, err - } return settings, nil } diff --git a/internal/commands/proc.go b/internal/commands/proc.go index 5a0bf8fd..22a43ab0 100644 --- a/internal/commands/proc.go +++ b/internal/commands/proc.go @@ -10,6 +10,7 @@ import ( "sync" "time" + "github.com/djdv/go-filesystem-utils/internal/generic" "github.com/djdv/p9/p9" "github.com/multiformats/go-multiaddr" ) @@ -156,9 +157,8 @@ func getListenersFromProc(ipc io.ReadWriteCloser, stderr io.ReadCloser, options if errs != nil { accumulateErr(client.Shutdown(patientShutdown)) } else if len(maddrs) == 0 { - errs = append(errs, fmt.Errorf( - "%w: daemon didn't return any addresses", - errServiceNotFound, + errs = append(errs, generic.ConstError( + "daemon didn't return any addresses", )) } accumulateErr(client.ipcRelease()) diff --git a/internal/commands/shared.go b/internal/commands/shared.go index 153d1e95..33f2608f 100644 --- a/internal/commands/shared.go +++ b/internal/commands/shared.go @@ -1,7 +1,5 @@ package commands -import "github.com/multiformats/go-multiaddr" - const ( // serverRootName defines a name which servers and clients may use // to refer to the service in namespace oriented APIs. @@ -51,11 +49,3 @@ const ( // value to the file. shutdownFileName = "shutdown" ) - -func defaultServerMaddr() multiaddr.Multiaddr { - maddrs, err := userServiceMaddrs() - if err != nil { - panic(err) - } - return defaultClientMaddr{Multiaddr: maddrs[0]} -} diff --git a/internal/commands/shutdown.go b/internal/commands/shutdown.go index 30641b86..d3a714a9 100644 --- a/internal/commands/shutdown.go +++ b/internal/commands/shutdown.go @@ -92,7 +92,7 @@ func (so shutdownOptions) make() (shutdownSettings, error) { if err := generic.ApplyOptions(&settings, so...); err != nil { return shutdownSettings{}, err } - return settings, settings.clientSettings.fillDefaults() + return settings, nil } func shutdownLevelsTable() string { diff --git a/internal/commands/unmount.go b/internal/commands/unmount.go index 1b8ac44b..834eb651 100644 --- a/internal/commands/unmount.go +++ b/internal/commands/unmount.go @@ -64,11 +64,7 @@ func (uo *unmountCmdOptions) BindFlags(flagSet *flag.FlagSet) { } func (uo unmountCmdOptions) make() (unmountCmdSettings, error) { - settings, err := makeWithOptions(uo...) - if err != nil { - return unmountCmdSettings{}, nil - } - return settings, settings.clientSettings.fillDefaults() + return makeWithOptions(uo...) } // Unmount constructs the command which requests the file system service