Re: [RFC PATCH v6 07/11] trace-cmd: Add `trace-cmd setup-guest` command

[Slavomir Kaslev <kaslevs@vmware.com>]

On Thu, Feb 14, 2019 at 03:41:32PM -0500, Steven Rostedt wrote:
> On Thu, 14 Feb 2019 16:13:31 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
> 
> > Add `trace-cmd setup-guest` command that creates the necessary FIFOs for tracing
> > a guest over FIFOs instead of vsockets.
> > 
> > Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
> > ---
> >  tracecmd/Makefile              |   1 +
> >  tracecmd/include/trace-local.h |   6 ++
> >  tracecmd/trace-cmd.c           |   1 +
> >  tracecmd/trace-setup-guest.c   | 186 +++++++++++++++++++++++++++++++++
> >  tracecmd/trace-usage.c         |   8 ++
> >  5 files changed, 202 insertions(+)
> >  create mode 100644 tracecmd/trace-setup-guest.c
> > 
> > diff --git a/tracecmd/Makefile b/tracecmd/Makefile
> > index 865b1c6..d3e3080 100644
> > --- a/tracecmd/Makefile
> > +++ b/tracecmd/Makefile
> > @@ -35,6 +35,7 @@ TRACE_CMD_OBJS += trace-msg.o
> >  
> >  ifeq ($(VSOCK_DEFINED), 1)
> >  TRACE_CMD_OBJS += trace-agent.o
> > +TRACE_CMD_OBJS += trace-setup-guest.o
> >  endif
> >  
> >  ALL_OBJS := $(TRACE_CMD_OBJS:%.o=$(bdir)/%.o)
> > diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
> > index 823d323..b23130e 100644
> > --- a/tracecmd/include/trace-local.h
> > +++ b/tracecmd/include/trace-local.h
> > @@ -14,6 +14,10 @@
> >  
> >  #define TRACE_AGENT_DEFAULT_PORT	823
> >  
> > +#define GUEST_PIPE_NAME		"trace-pipe-cpu"
> > +#define GUEST_DIR_FMT		"/var/lib/trace-cmd/virt/%s"
> > +#define GUEST_FIFO_FMT		GUEST_DIR_FMT "/" GUEST_PIPE_NAME "%d"
> > +
> >  extern int debug;
> >  extern int quiet;
> >  
> > @@ -68,6 +72,8 @@ void trace_listen(int argc, char **argv);
> >  
> >  void trace_agent(int argc, char **argv);
> >  
> > +void trace_setup_guest(int argc, char **argv);
> > +
> >  void trace_restore(int argc, char **argv);
> >  
> >  void trace_clear(int argc, char **argv);
> > diff --git a/tracecmd/trace-cmd.c b/tracecmd/trace-cmd.c
> > index 3ae5e2e..4da82b4 100644
> > --- a/tracecmd/trace-cmd.c
> > +++ b/tracecmd/trace-cmd.c
> > @@ -85,6 +85,7 @@ struct command commands[] = {
> >  	{"listen", trace_listen},
> >  #ifdef VSOCK
> >  	{"agent", trace_agent},
> > +	{"setup-guest", trace_setup_guest},
> >  #endif
> >  	{"split", trace_split},
> >  	{"restore", trace_restore},
> > diff --git a/tracecmd/trace-setup-guest.c b/tracecmd/trace-setup-guest.c
> > new file mode 100644
> > index 0000000..875ac0e
> > --- /dev/null
> > +++ b/tracecmd/trace-setup-guest.c
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 VMware Inc, Slavomir Kaslev <kaslevs@vmware.com>
> > + *
> > + */
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <getopt.h>
> > +#include <grp.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> > +
> > +#include "trace-local.h"
> > +#include "trace-msg.h"
> > +
> > +static int make_dir(const char *path, mode_t mode)
> > +{
> > +	char *buf, *end, *p;
> > +	int ret = 0;
> > +
> > +	buf = strdup(path);
> > +	end = buf + strlen(buf);
> > +
> > +	for (p = buf; p < end; p++) {
> > +		for (; p < end && *p == '/'; p++);
> > +		for (; p < end && *p != '/'; p++);
> 
> Why not:
> 	for (p = buf; p && *p; p++) {
> 
> 		for (; *p == '/'; p++);
> 		p = index(p, '/');
> 		if (p)
> 			*p = '\0';
> 			
> 
> > +
> > +		*p = '\0';
> > +		ret = mkdir(buf, mode);
> > +		if (ret < 0) {
> > +			if (errno != EEXIST) {
> > +				ret = -errno;
> > +				break;
> > +			}
> > +			ret = 0;
> > +		}
> 
> 		if (p)
> 			*p = '/';
> 
> ?
> 
> No need for "end".
> 
> -- Steve
> 
> > +		*p = '/';
> > +	}
> > +
> > +	free(buf);
> > +	return ret;
> > +}
> > +
> 

This won't work as proposed: `p` will be NULL on the last iteration but will
still get incremented from the outer for-loop and the check (p && *p) won't get
triggered (p == 0x01 in this case).

A fixed version might look like this:

static int make_dir(const char *path, mode_t mode)
{
	char buf[PATH_MAX+1], *p;
	int ret = 0;

	strncpy(buf, path, sizeof(buf));
	for (p = buf; *p; p++) {
		for (; *p == '/'; p++);
		p = strchr(p, '/');

		if (p)
			*p = '\0';
		ret = mkdir(buf, mode);
		if (ret < 0) {
			if (errno != EEXIST) {
				ret = -errno;
				break;
			}
			ret = 0;
		}
		if (!p)
			break;
		*p = '/';
	}

	return ret;
}

OTOH I find the original version much more readable:

static int make_dir(const char *path, mode_t mode)
{
	char buf[PATH_MAX+1], *end, *p;
	int ret = 0;

	end = stpncpy(buf, path, sizeof(buf));
	for (p = buf; p < end; p++) {
		for (; p < end && *p == '/'; p++);
		for (; p < end && *p != '/'; p++);

		*p = '\0';
		ret = mkdir(buf, mode);
		if (ret < 0) {
			if (errno != EEXIST) {
				ret = -errno;
				break;
			}
			ret = 0;
		}
		*p = '/';
	}

	return ret;
}

The intent behind `*p = '\0'; ... *p = '/';` is more clearly expressed in this
version without getting bogged down by strchr() edge case handling.

Since this is not on a performance critical path how about sticking to the more
readable of the two?

-Slavi