Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9P/CLI API refactoring #28

Merged
merged 194 commits into from
Jul 11, 2023
Merged

9P/CLI API refactoring #28

merged 194 commits into from
Jul 11, 2023

Conversation

djdv
Copy link
Owner

@djdv djdv commented Nov 26, 2022

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 599463f 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 #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 #27 originally derived from #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 djdv force-pushed the j/fs-impl-refactoring branch from 74a87e7 to 5c3542e Compare December 21, 2022 04:11
@djdv
Copy link
Owner Author

djdv commented Dec 28, 2022

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.

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 djdv force-pushed the j/fs-impl-refactoring branch from 957f536 to c8f7bf5 Compare April 3, 2023 06:39
@djdv djdv force-pushed the j/api-refactoring branch from 599463f to 4e31ddf Compare April 3, 2023 06:41
@djdv djdv force-pushed the j/fs-impl-refactoring branch from c8f7bf5 to bac3e78 Compare May 19, 2023 11:33
@djdv djdv force-pushed the j/fs-impl-refactoring branch from 1eb3315 to 748a288 Compare June 29, 2023 15:41
Base automatically changed from j/fs-impl-refactoring to staging/refactor July 11, 2023 14:50
djdv added 22 commits July 11, 2023 10:52
This made more sense when every file system had its own SetAttr method.
Since we embed this type now, we can just inline the behavior.

We also drop the condition which returns an error when
`P9_SETATTR_XTIME_SET` is provided, but `P9_SETATTR_XTIME` is not.
Documentation doesn't specify what to do in this case, so instead, we
now silently drop the provided time value in such a request.
This check may be added back in later if it seems appropriate to do so.
This is a series of commits that contained bulk changes that were squashed.
Some were proper enough to be their own commits but for the sake of
reducing intermediate diffs, they've been flattened into this one
alongside the others that lacked proper messages.

- listener: fix UDS missing parent in path assembler
- listener: update tests
- listener: add/fix rename support for maddr values
- directory: try to prevent orphaning tmpdir entries
Consider the sequence on an ephemeral directory:
`mkdir ed;cd ed;>file;rm file;cd ..`
`rm file` cascades UnlinkAt, causing "ed" to unlink from its parent.
In this sequence:
`mkdir ed;cd ed;>file;rm file;>file2;cd ..`
The creation of "file2" succeeds, but "ed" is now unreachable from
external references, and by proxy, so is "file2".

This change makes unlinking happen on the last close, instead of at the
time of UnlinkAt; so the above sequence will behave more like a
traditional directory during such a sequence.

Note that a directory FID must be held between creation, or an error
will occur.
E.g. `mkdir ed;>./ed/file;rm ./ed/file; >./ed/file2`
will fail as "ed" will not have a held reference after `rm ./ed/file`,
and thus become unlinked when the FID for "ed" is clunked.

- api: adapt to IPFS constructor changes
- api: remove hardcoded ID table (no longer used)
- mount: unbreak unmount

This is a bad hack, in the future we'll likely use table for this.
Probably `/mounts/index` similar to ACME.
- add `WithIdleDuration` option to the server
- add a `TrackedIO` interface
- - with an implementation constructor for `manet.Conn`s
- remove a magic cross pkg interface check
- fix some race conditions
- - with the server's listener+connection map
- - with updating timestamps on tracked connections
The `p9.ReadOnly` constant is `0`. In order to distinguish between Go's
zero value for integer types, we can use an additional bit beyond the
`p9.OpenFlagsModeMask` to denote that a file was successfully
initialized (from within the `Open` method); and mask this bit out when
doing access checks (from within `Read`, `Write`, etc.).
This is more complicated than the old one, but should allow constructors
to be more flexible in how they construct complex data types.
It's also harder to misuse. If the source code falls out of sync, these
functions will panic with messages explaining what's wrong.
If the Go compiler had support for common struct access in unions, this
would be a compile time error `x.y = z` "x has no field y".
Since it does not, option functions should have Go tests to assure these
panics won't occur at runtime outside of development builds.

Option types can be easily shared among different type constructors by
using a struct type for the option's parameter.
The struct may contain fields (directly or embedded) that
will be assigned to when the option func is called.
E.g. `type settings struct { fieldName fieldType }`
Alternatively, the struct may contain a slice of option funcs, which is 
appended to when the option func is called. This allows a constructor
to accept options for a subtype's constructor.
E.g. `type settings { suboptions []optionType }`
Both styles can be used on the same struct at the same time, and values
will be propagated to all targets they apply to.
I.e. if an option can set a field on the struct itself, and it can be
appended to a slice on the struct; the value will be duplicated and
applied to both.
djdv added 3 commits July 11, 2023 10:52
Some of our structs are large and would benefit from being passed via
pointers. These are all passed down and fields are copied before any
scope closures so these shouldn't escape the stack / get heap allocated.
This interface needs more consideration and is likely to change in the
future.
For now, we'll make it an optional extension.
Neither the host nor the guest need to support this, but if they do we
will handle the requests.
@djdv djdv force-pushed the staging/refactor branch from 4b264f5 to 8167f4c Compare July 11, 2023 14:53
@djdv djdv force-pushed the j/api-refactoring branch from 4e31ddf to 502f3e2 Compare July 11, 2023 14:53
djdv added 15 commits July 11, 2023 12:38
This makes no sense now that the library is extracted.
It's the users responsibility to guard against including this packages.
If a value's type name isn't provided in the usage string,
`flag.UnquoteUsage` provides a default name for specific types; these
types are unexported and local to the `flag` package.
Since external `flag.Value` implementations won't be matched by those
type checks, we can add a way to override the default name of "value".
This is specifically for `time.Duration` values, but generally applies
to any value type that's missing a name in the `usage` string.
Formerly a grouping, now a table. Unfortunately not a list.
Some `chmod` behavior is implementation defined (see: POSIX spec).
Since the 9P2000.L protocol is used, we test against GNU's `chmod`.
Trying to run these tests on Unix systems like Illumos, Darwin, et al.
will produce different results for the same symbolic expressions, as
they have different implementations / symbolic expression parsing rules.
@djdv djdv force-pushed the j/api-refactoring branch from 502f3e2 to bbe92d4 Compare July 11, 2023 16:38
djdv added 2 commits July 11, 2023 14:07
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.
@djdv djdv force-pushed the j/api-refactoring branch from bbe92d4 to 78a26a5 Compare July 11, 2023 20:07
@djdv djdv changed the base branch from staging/refactor to master July 11, 2023 20:08
@djdv
Copy link
Owner Author

djdv commented Jul 11, 2023

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.

@djdv djdv changed the base branch from master to staging/refactor July 11, 2023 20:23
@djdv djdv merged commit 8b5e240 into staging/refactor Jul 11, 2023
@djdv djdv deleted the j/api-refactoring branch July 11, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant