Re: [PATCH] pasta.c: modify hostname when detaching new namespace

Thanks for the review and the links.

+ if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX))) { + if (sethostname(hostname, strlen(hostname))) + debug("Unable to set pasta-prefixed hostname"); }

The above snippet, although it looks correct, doesn't work in cases where the hostname is long enough (~>58 chars). It works fine for shorter hostnames. The call to `gethostname()` sets errno to 36 (ENAMETOOLONG). Upon looking further, it seems that even though the manpage for `gethostname(char *name, size_t len)` states..

If the null-terminated hostname is too large to fit, then the name is truncated, and no error is returned (but see NOTES below).

...It would still throw ENAMETOOLONG if the value of len is smaller than `strlen(hostname)+1` (referring to the snippet below). I'm not sure if I'm missing out on an edge case while calculating the boundaries here because the `memcpy` call is certainly truncating the hostname[1] before ending up returning ENAMETOOLONG, which seems conflicting[1]:

int __gethostname (char *name, size_t len) { struct utsname buf; size_t node_len; if (__uname (&buf)) return -1; node_len = strlen (buf.nodename) + 1; memcpy (name, buf.nodename, len < node_len ? len : node_len); if (node_len > len) { __set_errno (ENAMETOOLONG); return -1; } return 0; }

[1] - https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/posix/gethost... On Tue, 21 May 2024 at 23:04, Stefano Brivio wrote:

On Mon, 20 May 2024 14:05:58 +0530 Danish Prakash wrote:

...

When invoking pasta without any arguments, it's difficult to tell whether we are in the new namespace or not leaving users a bit confused. This change modifies the host namespace to add a prefix "pasta-" to make it a bit more obvious.

Thanks for the patch, it works for me:

sbrivio@maja:~/passt$ ./pasta --config-net root@pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-past:~/passt#

a few comments (all about brevity and style), below:

...

Signed-off-by: Danish Prakash --- pasta.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/pasta.c b/pasta.c index 31e1e00..840a2b1 100644 --- a/pasta.c +++ b/pasta.c @@ -180,6 +180,8 @@ static int pasta_spawn_cmd(void *arg) { const struct pasta_spawn_cmd_arg *a; sigset_t set; + char hostname[HOST_NAME_MAX+1], pasta_hostname[HOST_NAME_MAX+1];

For coding style consistency: "HOST_NAME_MAX + 1" (with spaces).

...

+ char *hostname_prefix = "pasta-";

In passt, which follows the coding style of the Linux kernel for the networking part, we order variables from the longest to the shortest. Rationale:

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

see also https://lwn.net/Articles/758552/.

But actually, you don't need more than one variable here, see below.

...

/* We run in a detached PID and mount namespace: mount /proc over */ if (mount("", "/proc", "proc", 0, NULL)) @@ -188,6 +190,17 @@ static int pasta_spawn_cmd(void *arg) if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) warn("Cannot set ping_group_range, ICMP requests might fail");

+ if (gethostname(hostname, HOST_NAME_MAX+1) == 0) {

In general, unless one wants to stress that the return value could be a number of positive or negative values, we just use for the common case (0: success, -1: error):

if (!gethostname(...)) {

...

+ if ((strlen(hostname) + strlen(hostname_prefix)) > HOST_NAME_MAX) { + hostname[strlen(hostname)-strlen(hostname_prefix)] = '\0'; + }

For consistency: no curly brackets for statements that fit a single line.

...

+ sprintf(pasta_hostname, "%s%s", hostname_prefix, hostname);

You could drop all this if you initialise the target variable directly, say:

char hostname[HOST_NAME_MAX + 1] = "pasta-";

then you can gethostname() on it + sizeof("pasta-") - 1 (NULL terminating byte returned by sizeof()), directly.

Using a #define for "pasta-":

gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX));

...

+ + if (sethostname(pasta_hostname, strlen(pasta_hostname)) != 0) {

Same here about !sethostname() vs. sethostname() != 0, and curly brackets.

...

+ warn("Unable to set pasta-prefixed hostname"); + } + } + /* Wait for the parent to be ready: see main() */ sigemptyset(&set); sigaddset(&set, SIGUSR1);

-- Stefano