Re: [PATCH 06/10] Replace FWRITE with a function

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 11 Oct 2022 16:40:14 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In a few places we use the FWRITE() macro to open a file, replace it's
> contents with a given string and close it again.  There's no real
> reason this needs to be a macro rather than just a function though.

Well, there's a bit of a reason: it gives more descriptive messages in
isolate_user() with the same LoCs.

On the other hand I would also prefer a function here, probably better
than the alternative I'm not sure why this series needs this by the
way.

> Turn it into a function 'write_file()' and make some ancillary
> cleanups while we're there:
>   - Add a return code so the caller can handle giving a useful error message
>   - Handle the case of short write()s (unlikely, but possible)
>   - Add O_TRUNC, to make sure we replace the existing contents entirely
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  isolation.c | 17 +++++++++--------
>  pasta.c     |  4 ++--
>  util.c      | 33 +++++++++++++++++++++++++++++++++
>  util.h      | 13 +------------
>  4 files changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/isolation.c b/isolation.c
> index 10cef05..4aa75e6 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -129,7 +129,8 @@ void isolate_initial(void)
>   */
>  void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns)
>  {
> -	char nsmap[BUFSIZ];
> +	char uidmap[BUFSIZ];
> +	char gidmap[BUFSIZ];
>  
>  	/* First set our UID & GID in the original namespace */
>  	if (setgroups(0, NULL)) {
> @@ -184,14 +185,14 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns)
>  	}
>  
>  	/* Configure user and group mappings */
> -	snprintf(nsmap, BUFSIZ, "0 %u 1", uid);
> -	FWRITE("/proc/self/uid_map", nsmap, "Cannot set uid_map in namespace");
> +	snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> +	snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
>  
> -	FWRITE("/proc/self/setgroups", "deny",
> -	       "Cannot write to setgroups in namespace");
> -
> -	snprintf(nsmap, BUFSIZ, "0 %u 1", gid);
> -	FWRITE("/proc/self/gid_map", nsmap, "Cannot set gid_map in namespace");
> +	if (write_file("/proc/self/uid_map", uidmap) ||
> +	    write_file("/proc/self/setgroups", "deny") ||
> +	    write_file("/proc/self/gid_map", gidmap)) {
> +		warn("Couldn't configure user namespace");

It's unlikely that we can write to uid_map but not to setgroups. Still,
having separate messages, as we had them, could help investigating some
issues.

> +	}
>  }
>  
>  /**
> diff --git a/pasta.c b/pasta.c
> index 4ff322c..0ab2fe4 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -167,8 +167,8 @@ static int pasta_setup_ns(void *arg)
>  	const struct pasta_setup_ns_arg *a
>  		= (const struct pasta_setup_ns_arg *)arg;
>  
> -	FWRITE("/proc/sys/net/ipv4/ping_group_range", "0 0",
> -	       "Cannot set ping_group_range, ICMP requests might fail");
> +	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> +		warn("Cannot set ping_group_range, ICMP requests might fail");
>  
>  	execvp(a->exe, a->argv);
>  
> diff --git a/util.c b/util.c
> index 6b86ead..93b93b1 100644
> --- a/util.c
> +++ b/util.c
> @@ -547,3 +547,36 @@ int fls(unsigned long x)
>  
>  	return y;
>  }
> +
> +/**
> + * write_file() - Replace contents of file with a string
> + * @path:	File to write
> + * @buf:	String to write
> + *
> + * Return: 0 on success, -1 on any error
> + */
> +int write_file(const char *path, const char *buf)

We could also use this for the PID file by optionally taking a file number, but
I haven't tried how it looks like.

> +{
> +	int fd = open(path, O_WRONLY | O_TRUNC | O_CLOEXEC);
> +	size_t len = strlen(buf);
> +
> +	if (fd < 0) {
> +		warn("Could not open %s: %s", path, strerror(errno));
> +		return -1;
> +	}
> +
> +	while (len) {
> +		ssize_t rc = write(fd, buf, len);
> +
> +		if (rc < 0) {

I would change this to <= 0. Not that it matters with write(), but
should we ever change that another function, we run the (absolutely
not critical) risk of forgetting to adjust this and get stuck in a loop
here.

> +			warn("Couldn't write to %s: %s", path, strerror(errno));
> +			break;
> +		}
> +
> +		buf += rc;
> +		len -= rc;
> +	}
> +
> +	close(fd);
> +	return len == 0 ? 0 : -1;
> +}
> diff --git a/util.h b/util.h
> index 0c06e34..f957522 100644
> --- a/util.h
> +++ b/util.h
> @@ -58,18 +58,6 @@ void trace_init(int enable);
>  #define TMPDIR		"/tmp"
>  #endif
>  
> -#define FWRITE(path, buf, str)						\
> -	do {								\
> -		int flags = O_WRONLY | O_CLOEXEC;			\
> -		int fd = open(path, flags);				\
> -									\
> -		if (fd < 0 ||						\
> -		    write(fd, buf, strlen(buf)) != (int)strlen(buf))	\
> -			warn(str);					\
> -		if (fd >= 0)						\
> -			close(fd);					\
> -	} while (0)
> -
>  #define V4		0
>  #define V6		1
>  #define IP_VERSIONS	2
> @@ -215,5 +203,6 @@ int ns_enter(const struct ctx *c);
>  void write_pidfile(int fd, pid_t pid);
>  int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);
> +int write_file(const char *path, const char *buf);
>  
>  #endif /* UTIL_H */

-- 
Stefano