feat: match p9p namespace calculation by respecting NAMESPACE and DISPLAY #62
Conversation
Since this is unix specific, I'm basing this on 9p(1), namespace(1) and intro(4) from plan9port. - The namespace is the /tmp/ns.$USER.$DISPLAY directory that consists of unix sockets to various 9p servers (without any nesting). - This can be used by e.g. 9p(1) to create the illusion of a coherent namespace where you can 9p read foo/bar which actually dispatches to the singular /tmp/ns.$USER.$DISPLAY/foo socket. I've called the first path component, which always needs to be given `server_name`. I don't know if it has a more specific term in plan9port, though. My main aim is to prevent a confusion with namespace which (in plan9port) refers to the directory that contains the 9p unix sockets.
The behavior should not have changed yet, but this gives us the ability to implement support for NAMESPACE in a central place.
The use of String for most things 9p is a good fit since its messages use UTF-8 encoding. The namespace is a POSIX path, though, and may theoretically be an arbitrary byte sequence, so it's better to use PathBuf (and OsString). Doing this consistently in ninep should also create a nice distinction between 9p paths and POSIX paths via their types. Additionally, we no longer need to duplicate logic between socket_path() and unix_socket() in server because we can use PathBuf::parent().
Match plan9port's behavior for determining the namespace:
- If NAMESPACE is set, use its value.
- Otherwise, use a combination of username and the value of the
DISPLAY variable.
- Remaining divergences:
- ninep falls back to :0 if DISPLAY is unset.
- ninep uses the USER environment variable for determining the user
name whereas plan9port uses getuid(3).
This means that 9p(1) and ad will behave the same now, unless $USER and getuid() diverge.
|
@sternenseemann as with the previous PR you opened, please can you raise issues to discuss changes like this rather than opening a PR with a breaking change like this. Particularly for anything around the 9p support, if what you are after is compatibility with other implementations then that is likely going to be quite involved as the current support is pretty minimal. I don't currently have any contribution guidelines for this repo but that is largely because (as stated quite clearly at the start of the readme) this project is very much in its early stages and not really something that is suitable for external contribution. I'll add some more explicit details about this today to make that clearer. |
Can you clarify what breaking change you have a problem with. I'm happy to redesign this change in a way that the API of the The behavorial changes are unproblematic in my opinion because in cases where it'd cause
All I wanted to implement with this change is to match the behavior of What I'm personally after is that I can just set |
|
I'm just referring to the changes you listed in your PR description. The ninep one around pathbufs isn't too bad as you have pointed out as there are multiple ways to modify what you have here so that it isn't a breaking change to the public API, but Having the In principal I'm not against a change like this (accounting for the concerns I've outlined above) but what I'm not keen on is PRs where what is being proposed is a change to the codebase without any explanation of why that change is needed / desired.
This is something that makes sense to me and something that I'll happily discuss in an issue if support for Wayland and compatibility with existing 9p clients is what you are after. As I have mentioned previously and as is hopefully made clear in the README, this project is very much expected to have numerous rough edges at the moment. I don't currently have things set up in a way that makes it clear to others what the ongoing work I'm doing is looking like or what aspects of the design I am / am not open to modifying at the moment. All I'm asking is that changes beyond simple bugfixes (such as #59) as first raised as issues for discussion rather opened as PRs. I hope this all makes sense 🙂 |
That can easily be changed by dropping c51eb5b from this PR.
I figured it would not be a big deal since we already need I did have the idea that |
With this PR, the ninep crate should default to the same namespace as plan9port utilities in all cases, i.e. match
getns.c.This entails the following, technically breaking, changes
adwill now crash on startup ifDISPLAY(and alsoNAMESPACE) is not set. This could be made a bit nicer by threading the error through which I haven't done yet.PathBufin some places now. This is an API breaking change, so probably needs a semver bump. I don't know how this is managed at the moment, so I haven't done anything on this front.mount-adneeds thenamespace(1)utility from plan9port additionally now.