djdv / go-filesystem-utils

ISC License
10 stars 2 forks source link

9P/CLI API refactoring #28

Closed djdv closed 1 year ago

djdv commented 1 year ago

This is not the 9P host or client code which will be ported over later. Although these features are likely going to share a lot of code.

These are the various changes made to our 9P API, which is what our CLI uses to do RPC with the fs daemon. Effectively all the pluming to take user input and translate it into requests for our system constructors, mappers, and managers.

As an aside, it's also possible to attach to the 9P API from any 9P2000.L compliant client (such as v9fs in the Linux kernel) to make the same requests our CLI program does, except by hand/shell. (The API for that is not documented or stable yet though.) You can use fs daemon -verbose to see what socket it's listening on, or provide one with fs daemon -server $muliaddr. fs daemon -help for the rest of the options.


TODO:

I'm not sure if 599463f199b9c73aae3f4350d5011ea8f5f69528 deserves to stay or not. It's here just to make sure the tip of this branch passes CI. If so, we have to put it back in the first set, or factor it out and just wait until 1.20 lands (and then completely tackle with the new features from standard errors https://github.com/djdv/go-filesystem-utils/issues/22).

We might want to make sure all the commands have their descriptions filled in before this gets merged. While flags are generally well defined, there's a lot of "Placeholder text" for commands themselves. Although their names do make them pretty obvious. You can imaging what fs mount might be used for, without a description. But examples and the like would be nice to see in the helptext.

Since things got split up and re-ordered for PRs, I need to go through each commit 1 by 1 and run go mod tidy on them, just to make sure things get added with the commits they belong to. Things still aren't going to build but it would be nice to have the history/blame for dependencies line up. We can do this last because it will rewrite commit hashes, and generally ruins GitHub review comments as a result. Likewise with splitting. If something should be split, we can just make a remark about it and do it last for the same reason.


PR meta: Adds on top of https://github.com/djdv/go-filesystem-utils/pull/27 originally derived from https://github.com/djdv/go-filesystem-utils/pull/20 Initially targeting j/fs-impl-refactoring, which should target staging/refactor after that's merged.

This is the final set from our latest refactoring pass. Diffing against j/ipfs-fs-extensions should only show dependency updates and the CI shushing. Until we start making new changes.

djdv commented 1 year ago

This post isn't important, just notes on plans and thoughts while refactoring.


I wrote a generic function to handle Twalk requests, which saved a lot of duplicate code across the various 9P file systems and their interlinking patterns. However, the interface is not as simple as it probably could be, and easy to silently misuse, at least when composing types. Specifically, we might be calling embedded methods when not intending to, which compiles and works, but returns the wrong type which satisfies the same base interface. E.g. Super.Walk(nil) (expecting to return type "Super"), may accidentally return from Super.sub.Walk(nil) (actually returns type "sub") if we forget to explicitly define Super.Walk. And other various traps like that.

I'm going to see if it makes sense to keep it, refactor it, or just make sure each system implements their own type-specific variants of the Walk method. The last of which is probably the most sensible since everything should be validated more strictly by the compiler, and reduces some of the method inheritance nightmare currently going on.


In addition, the functional options I have for this pkg are pretty gross, and make the constructors pretty horrid. The goal was to be able to share common settings fields + option names, across the constructed types. So a single WithID option that can be used with multiple constructors, instead of WithIDListener, WithIDMounter, ... This spawned some converter functions out of necessity too, which take a list of options for a subtype and allow them to be combined with options for a supertype's constructor. WithSuboptions[SuperOptions](subOptions...) This is a neat trick, but awkward and not nice imo.

Example of monster as it's currently used in the daemon init: Here we're creating 3 different file systems and linking them together, sharing any overlapping options. ``` go func newSystems(uid p9.UID, gid p9.GID, netCallback p9fs.ListenerCallback) (*p9fs.Directory, *p9fs.Listener) { const permissions = p9fs.ReadUser | p9fs.WriteUser | p9fs.ExecuteUser | p9fs.ReadGroup | p9fs.ExecuteGroup | p9fs.ReadOther | p9fs.ExecuteOther var ( metaOptions = []p9fs.MetaOption{ p9fs.WithPath(new(atomic.Uint64)), p9fs.WithBaseAttr(&p9.Attr{ Mode: permissions, UID: uid, GID: gid, }), p9fs.WithAttrTimestamps(true), } directoryOptions = []p9fs.DirectoryOption{ p9fs.WithSuboptions[p9fs.DirectoryOption](metaOptions...), } _, fsys = p9fs.NewDirectory(directoryOptions...) generatorOptions = []p9fs.GeneratorOption{ p9fs.CleanupEmpties(true), } mounter = p9fs.NewMounter( p9fs.WithSuboptions[p9fs.MounterOption](metaOptions...), p9fs.WithSuboptions[p9fs.MounterOption]( p9fs.WithParent(fsys, mounterName), ), p9fs.WithSuboptions[p9fs.MounterOption](generatorOptions...), ) _, listeners = p9fs.NewListener(netCallback, p9fs.WithSuboptions[p9fs.ListenerOption](metaOptions...), p9fs.WithSuboptions[p9fs.ListenerOption]( p9fs.WithParent(fsys, listenerName), ), p9fs.WithSuboptions[p9fs.ListenerOption](generatorOptions...), ) ) for _, file := range []struct { p9.File name string }{ { name: mounterName, File: mounter, }, { name: listenerName, File: listeners, }, } { if err := fsys.Link(file.File, file.name); err != nil { panic(err) } } return fsys, listeners } ```

Some of the Go generic proposals would help make this cleaner, both to call and to implement. But as-is, those are still being discussed and not likely to come soon.

I'll try to look at these again and consider another approach. Either restructuring things within this pkg, or splitting the packages up and just accepting duplication across different namespaces (listener.WithID, mounter.WithID). For the former, there may be some other way the options could be formatted. Either as some interface with methods, or just a different way of writing the generic option generator + constructors.

djdv commented 1 year ago

I must have messed up a rebase somewhere. Rather than merging directly into master, I'm just going to merge this into the staging branch, then replace master with that. I'm confident nobody is building on top of the current master reference, and all that code is being blown away by this anyhow. Will try not to do things like that in the future.

This was reviewed and tested mostly offline. It likely still has issues in places, but is good enough to move forward with.