search

hugelgupf / p9

Idiomatic Go 9P2000.L client and server, extracted from gVisor for general use
Apache License 2.0
91 stars 19 forks source link

gVisor extensions: yay or nay? #80

Open hugelgupf opened 1 year ago

hugelgupf commented 1 year ago

p9 code base currently has several gVisor protocol extensions (9P2000.L.Google.N, where N is some monotonically increasing number denoting support for ).

This support is only exercised between the client and server present in this code base. It would likely also have worked with gVisor before gVisor removed 9P support (I think it's all lisafs only now, on the public side).

Is it worth keeping some of the extensions?

T/Rwalkgetattr is mainly a bit annoying for server implementers at the moment, though p9.DefaultWalkGetAttr provides some relief. I think on the creation side, we currently do not handle UIDs or GIDs correctly for 9P2000.L at all, in any sense of the word. (Though removal of the extension may not necessarily help improve that situation.)

hugelgupf commented 1 year ago

cc @djdv @rminnich

djdv commented 1 year ago

Short version:

Leaning towards nay. But I can see a possibility of supporting both at once. When it comes to variants I'm more of a fan of the 9P2000 base spec than any of the other extensions (.u, .L, .L.Google) when interop with the OS isn't a concern.

Long version:

I wonder if it would make sense to have both as separate interfaces. 1 interface could define the standard .L File and the other could embed+extend it for something like GVFile. The server could then check a File value's type to see if it should advertise as .L or .L-Google, and do the same when handling messages. That might be too complicated to be worth it though. Especially if the extensions aren't likely to be widely used on either the client or server side of things (as opposed to programs using just the base .L).

(Note that I haven't really used this library extensively so the following statement should hold less weight)

The rationale for WalkGetattr makes sense, but I don't think I've ever called it. The only attribute fields I tend to care about right now are the mode and sometimes the size, but when making protocols on top of 9P I tend to just depend on implementations like Read and Readdir to fail if there's some kind of file-type mismatch, rather than checking for it ahead of time. I.e. I expect Read to fail if I accidentally call it on a directory, same with Readdir on a regular file. Instead of checking the mode before calling the operation.

That said, I bet that pattern in my code would change if I had more reasons to use fields like the UID and GID parts of the attr. Without support for things like the auth message, in addition to the Attach method of the interface omitting things like the uname, I have yet to focus on any kind of security enforcment in the things I've built with this library. But that's not by choice, I'd rather have these things so that I can comfortably expose a service to the public internet and have some semblance of auth and permission/capability enforcement.

In regards to the create messages carrying UID I don't have any strong opinions on that at this time. I know I put thought into it before but don't remember all the details in regards to how the original 9P protocol handled it, and how POSIX + Linux handle this in their syscalls + .L. I suppose it could be useful, but I know the base 9P spec has some implied inheritance rules for this, and since setattr exists, you could just utilize it if you needed to deviate from that inheritance pattern. i.e. Create a file then immediately setattr the uid. But I could also see the benefit of doing that in a single call/request.

As an aside to this, if the interface is going to have to change at some point anyway, I'd like to see Walk become variadic instead of requiring a slice type. I.e. Walk(names ...string) instead of Walk(names []string) Most of the time I'm either cloning a fid with Walk(nil) or walking down a single name that I already have as a string which forces wrapping it manually Walk([]string{nameValue}) when it could be Walk(nameValue). Existing slices can still be passed by just using the ellipses Walk(namesSlice...).

Also in regards to protocol variants, even the .L extension itself is kind of annoying when compared to the original 9P2000 spec, but at least for Linux+POSIX interop we're kind of stuck with it. But in situations where that's not the goal, I'd much rather be writing things using the base 9P spec than the .u, .L, or .L.Google variants.