-
Notifications
You must be signed in to change notification settings - Fork 40
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
conn: do not chdir for conn.sock #581
base: develop
Are you sure you want to change the base?
Conversation
Instead, we create the socket in a temporary directory - TMPDIR if it is not too long, or else /tmp. The directory is made via `mkdtemp` with the template `urbit-XXXXXX`. The socket is located there (getting around the length limit) and `$pier/.urb/conn.sock` is symlinked to it. Replaces calls to uv_strerror with strerror for system calls; the former does not print system call errors properly.
The pier socket link is predictable; no information is conveyed by printing it.
I'm actually indifferent as to whether this or #580 is taken. I think this is a slightly better solution, since it allows clients to always be able to specify the full absolute path to the socket when connecting to it. But either way is fine. |
@pkova Are you the right person to review this? |
Now uv_close is never needed as part of failure cleanup. N.B. this can be moved before the close to keep the cleanup logic stacked.
Also handles 0-length TMPDIR.
Oh, I guess now there's also In that case the temporary directory should probably be stored on |
@mopfel-winrux how do you want to handle If not, then the next best thing is probably to reopen #580 and also apply that to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but flagged one thing
} | ||
memcpy(tad_c + len_w + sizeof("/urbit-XXXXXX") - 1, "/conn.sock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string "/urbit-XXXXXX" gets reused a lot, we should make it a constant or macro
@mrdomino I'm sorry I completely missed this. We should also do this for |
Yeah. How does lick work? Does it make one socket or many?
I guess it's going to be necessary to track and manage the temp dir from some location that is neither conn.c nor lick.c if we go this route.
I also gave a little thought to putting the socket directory under /run/urbit/sampel-palnet rather than /tmp/urbit-XXXXXX. I'm leaning towards /tmp at the moment, but could be persuaded either way.
…On Saturday, March 2nd, 2024 at 17:56, mopfel-winrux ***@***.***> wrote:
***@***.***(https://github.com/mrdomino) I'm sorry I completely missed this. We should also do this for lick.c
—
Reply to this email directly, [view it on GitHub](#581 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAAFT2ZB4P35LA3JYJEJTGLYWJ7NRAVCNFSM6AAAAABBNZHUO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZUHE3TOOJSGQ).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
It makes multiple sockets. One per |
Ok, so supposing we want to keep sockets symlinked in the pier directory, maybe we can just do a symlink from dev under the temp directory to dev under the pier directory?
…On Monday, March 11th, 2024 at 06:32, mopfel-winrux ***@***.***> wrote:
It makes multiple sockets. One per [%spin name=path] card it is given. It puts them in `.urb/dev/ folder under the name path. we could use that directory
—
Reply to this email directly, [view it on GitHub](#581 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAAFT2Y4ROJDESATN7D2HJ3YXW57LAVCNFSM6AAAAABBNZHUO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBYGQ2TCNJSGQ).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Instead, we create the socket in a temporary directory -
TMPDIR
if it is not too long, or else/tmp
. The directory is made viamkdtemp
with the templateurbit-XXXXXX
. The socket is located there (getting around the length limit) and$pier/.urb/conn.sock
is symlinked to it.This fixes an issue where if urbit is started with
sudo -u pieruser
from a directory that the pier user doesn't have access to (e.g. a different user's home directory that is chmod 0750), the process would die trying tochdir
back to the old directory.It also gives clients more flexibility; previously, they would have had to
chdir
to the pier directory to connect to the socket without risking overrunning the length limit. They can still do that (and use".urb/conn.sock"
as theirsun_path
), but they now also have the option of usingreadlink
and connecting to the full absolute path of the socket without changing directories.Replaces calls to
uv_strerror
withstrerror
for system calls; the former does not print system call errors properly.Supersedes #580.