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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > + *
> > + */
> > +
> > +#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
|