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

[Danish Prakash <contact@danishpraka.sh>]

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/gethostname.c;hb=HEAD


On Tue, 21 May 2024 at 23:04, Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Mon, 20 May 2024 14:05:58 +0530
> Danish Prakash <contact@danishpraka.sh> 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 <contact@danishpraka.sh>
> > ---
> >  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
>