conn_sock: add chdir-attach option #226
Conversation
|
fixes #225 |
|
go fmt is not happy. |
|
LGTM |
haircommander
force-pushed
the
chdir-attach
branch
from
41ea606 to
665ed3f
Compare
src/conn_sock.c
Outdated
| @@ -188,7 +202,7 @@ static char *bind_unix_socket(char *socket_relative_name, int sock_type, mode_t | |||
| if (fchmod(socket_fd, perms)) | |||
| pexitf("Failed to change socket permissions %s", sock_fullpath); | |||
|
|
|||
| if (chdir(opt_socket_path) == -1) | |||
| if (do_chdir && chdir(opt_socket_path) == -1) | |||
There was a problem hiding this comment.
Could we use the same trick with O_PATH I've used for Podman so we don't need a new flag and avoid using chdir?
There was a problem hiding this comment.
I think that's a good trick for the do_chdir case, but we still need this PR to maintain backwards compatibility with podman (that truncates the path once it gets long enough)
There was a problem hiding this comment.
acutally, that's a good trick for both cases, but we still will need to truncate the path sometimes, thus still need this flag
There was a problem hiding this comment.
don't we need to teach podman how to use the new flag though? How will it work with older versions?
There was a problem hiding this comment.
nevermind my last comment.
For backward compatibility could we bump the API version and use the full path version only when the API version is 2?
There was a problem hiding this comment.
jw, is there a reason you're opposed to a flag here? we could use api-version here, but it seems like a very small change to bump the conmon major version. and if we did, we'd still need a change in podman to actually absorb the change, which is no different than a flag
There was a problem hiding this comment.
the main reason is that I'd prefer if possible to avoid chdir as it affects the entire process
There was a problem hiding this comment.
okay I dropped chdir but kept a flag. Now we gate the behavior of truncating the path on a flag, but regardless don't chdir
haircommander
force-pushed
the
chdir-attach
branch
from
665ed3f to
ffc2cde
Compare
unfortunately, older podman versions rely on conmon truncating the unix socket length when setting up the attach socket. Introduce a flag to gate the changed behavior, allowing newer podman versions to be able to handle longer UID in rootless mode, but also allowing older podman versions to work. This also removes the logic for `do_chdir` in favor of using a /proc/self/fd entry for the sock path. That allows conmon to not chdir before attaching to the unix socket. Signed-off-by: Peter Hunt <pehunt@redhat.com>
haircommander
force-pushed
the
chdir-attach
branch
from
ffc2cde to
9add337
Compare
unfortunately, older podman versions rely on conmon truncating the unix socket length when setting up the attach socket.
Introduce a flag to gate the changed behavior, allowing newer podman versions to be able to handle longer UID in rootless mode,
but also allowing older podman versions to work.
Signed-off-by: Peter Hunt pehunt@redhat.com