From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-15.pe-b.jellyfish.systems (out-15.pe-b.jellyfish.systems [198.54.127.81]) by passt.top (Postfix) with ESMTPS id 8DBB45A0304 for ; Thu, 23 May 2024 15:52:59 +0200 (CEST) Received: from output-router-5c9bf9d745-bgrqh (new-01.privateemail.com [198.54.118.220]) by pe-b.jellyfish.systems (Postfix) with ESMTPA id 4VlV460MJ1zGpVZ for ; Thu, 23 May 2024 13:52:54 +0000 (UTC) Received: from MTA-12.privateemail.com (unknown [10.50.14.28]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by NEW-01.privateemail.com (Postfix) with ESMTPS id F1E1F18000D0 for ; Thu, 23 May 2024 09:52:53 -0400 (EDT) Received: from mta-12.privateemail.com (localhost [127.0.0.1]) by mta-12.privateemail.com (Postfix) with ESMTP id CCC4918000CD for ; Thu, 23 May 2024 09:52:53 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=danishpraka.sh; s=default; t=1716472373; bh=lgMsYgEDkFIPTZmM+NMiW/b6DbFt6/rIVc6ez2rpPFQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=GtVKV/k9TKbXOl8SMsRrngMUQoeGactSoGFxXV9f8HvXyhQOZ53oUHS+PqWRa1pmt rdDJTLoZlgQlxh/+ARwSGyAGDzXPPMskyFCxQLGs+qd4RLo7g/1YCLM2E8Ufib/9Tu pbUg4e1ASa0V4rIDNVe74JUauiPNYOA3GttsepDKOJ9Zo1Yt4EWKizr45v6jI43b0u vsAW91SjT0WhvD/GvLpxlFOX0tg4fslUZwkwscctr57NxIvL5SspnT0k9U8yUkiCDl FsWw3b0lv35/Gr1XSKC9t1PBWr+vpvcL37hVSyseZYHU8wxIQHJon1Mq18WSD7QFRL rIibw+7kpisjA== Received: from mail-il1-f182.google.com (mail-il1-f182.google.com [209.85.166.182]) by mta-12.privateemail.com (Postfix) with ESMTPA for ; Thu, 23 May 2024 09:52:53 -0400 (EDT) Received: by mail-il1-f182.google.com with SMTP id e9e14a558f8ab-36e9661e0feso19648765ab.3 for ; Thu, 23 May 2024 06:52:53 -0700 (PDT) X-Gm-Message-State: AOJu0YxEjifI3PrSOfrtf8UxGfzVfn6sxr0M/uLvsFh8/pkEUhph4K76 HE2hIruAEL/3pPsaPrk3qhk8P/y7d76l7jN3GJFhKYvQisyD98YimWjrclyXJDGBORaaI0lMERf pH5gzIB1UOz6ljZrAS2gxBfPYPPc= X-Google-Smtp-Source: AGHT+IEtjF2uWPLlUOQB+PIO40YDzHaAEo8ipJWhkfDVL/idIEfUTrxYjCT0D8YRUJKGaBFYKqbeopftYLAV45el2LM= X-Received: by 2002:a05:6e02:1523:b0:36c:559e:755f with SMTP id e9e14a558f8ab-371f7d753c9mr52063915ab.1.1716472373130; Thu, 23 May 2024 06:52:53 -0700 (PDT) MIME-Version: 1.0 References: <20240520083650.12032-1-contact@danishpraka.sh> <20240521193400.4f1b15c5@elisabeth> In-Reply-To: <20240521193400.4f1b15c5@elisabeth> From: Danish Prakash Date: Thu, 23 May 2024 19:22:16 +0530 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] pasta.c: modify hostname when detaching new namespace To: Stefano Brivio Content-Type: text/plain; charset="UTF-8" X-Virus-Scanned: ClamAV using ClamSMTP X-MailFrom: contact@danishpraka.sh X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation Message-ID-Hash: N6EURUQVOFANKGY3MHZYSCAWBHQ732IF X-Message-ID-Hash: N6EURUQVOFANKGY3MHZYSCAWBHQ732IF X-Mailman-Approved-At: Thu, 23 May 2024 16:03:28 +0200 CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 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 >