diff mbox series

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

Message ID 20190214141335.28144-8-kaslevs@vmware.com (mailing list archive)
State Superseded
Headers show
Series Add VM kernel tracing over vsockets and FIFOs | expand

Commit Message

Slavomir Kaslev Feb. 14, 2019, 2:13 p.m. UTC
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

Comments

Steven Rostedt Feb. 14, 2019, 8:41 p.m. UTC | #1
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;
> +}
> +
Slavomir Kaslev Feb. 18, 2019, 2:37 p.m. UTC | #2
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
Steven Rostedt Feb. 18, 2019, 5:44 p.m. UTC | #3
On Mon, 18 Feb 2019 16:37:47 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> 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).

I still don't like the "end", it just looks awkward.

> 
> 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?
> 

I'd still like to use '*p' as that's very common.

Also break up the other for loops into a while loops.

	for (p = buf; *p; p++) {

		while (*p == '/')
			p++;
		while (*p && *p != '/')
			p++;

		if (*p)
			*p = '\0';
		else
			p--; /* for the for loop */

		[...]


This would work, and I think is still readable. It matches more the
standard way of the Linux kernel as well.

-- Steve
Slavomir Kaslev Feb. 18, 2019, 8:07 p.m. UTC | #4
On Mon, Feb 18, 2019 at 12:44:37PM -0500, Steven Rostedt wrote:
> On Mon, 18 Feb 2019 16:37:47 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> > 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).
>
> I still don't like the "end", it just looks awkward.

If that's the only argument I don't think it stands.

>
> >
> > 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?
> >
>
> I'd still like to use '*p' as that's very common.

Testing for '*p' is more common since in the common case one doesn't know the
length of the string.

This is not the case here since we first do a copy anyway and hence we know the
length from then on. We also actively manipulate to string sentinel and knowing
where the string actually ends makes reasoning about the code much easier.

>
> Also break up the other for loops into a while loops.

OK switching the for()s to while()s and dropping the first `p < end` check
(which is never true) sounds fine.

>
> 	for (p = buf; *p; p++) {
>
> 		while (*p == '/')
> 			p++;
> 		while (*p && *p != '/')
> 			p++;
>
> 		if (*p)
> 			*p = '\0';
> 		else
> 			p--; /* for the for loop */
>
> 		[...]
>
>
> This would work, and I think is still readable.

It's really not more readable and having a comment explaining what's going on
only supports this claim.

That being said, I don't have strong feelings one way or another though,
disappointingly, I did want to share my bikeshedding opinion. It takes less
energy to make the proposed change than to explain why imo it's worse. If none
of the above makes any sense or you insist on dropping `end`, I'll send an
updated version of this patch for the v7 patchset tomorrow.

Thanks,

-- Slavi
Slavomir Kaslev Feb. 18, 2019, 8:54 p.m. UTC | #5
On Mon, Feb 18, 2019 at 10:07:06PM +0200, Slavomir Kaslev wrote:
> On Mon, Feb 18, 2019 at 12:44:37PM -0500, Steven Rostedt wrote:
> > On Mon, 18 Feb 2019 16:37:47 +0200
> > Slavomir Kaslev <kaslevs@vmware.com> wrote:
> >
> > > 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).
> >
> > I still don't like the "end", it just looks awkward.
> 
> If that's the only argument I don't think it stands.
> 
> >
> > >
> > > 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?
> > >
> >
> > I'd still like to use '*p' as that's very common.
> 
> Testing for '*p' is more common since in the common case one doesn't know the
> length of the string.
> 
> This is not the case here since we first do a copy anyway and hence we know the
> length from then on. We also actively manipulate to string sentinel and knowing
> where the string actually ends makes reasoning about the code much easier.
> 
> >
> > Also break up the other for loops into a while loops.
> 
> OK switching the for()s to while()s and dropping the first `p < end` check
> (which is never true) sounds fine.
> 
> >
> > 	for (p = buf; *p; p++) {
> >
> > 		while (*p == '/')
> > 			p++;
> > 		while (*p && *p != '/')
> > 			p++;
> >
> > 		if (*p)
> > 			*p = '\0';
> > 		else
> > 			p--; /* for the for loop */
> >
> > 		[...]
> >
> >
> > This would work, and I think is still readable.
> 
> It's really not more readable and having a comment explaining what's going on
> only supports this claim.
> 

I thing in the end we're comparing this:

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

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

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

	return 0;
}

to this version:

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

	strncpy(buf, path, sizeof(buf));
	for (p = buf; *p; p++) {
		bool eos = true;

		while (*p == '/')
			p++;
		while (*p && *p != '/')
			p++;

		if (*p)
			*p = '\0';
		else
			eos = false;
		if (mkdir(buf, mode) < 0 && errno != EEXIST)
			return -errno;
		if (eos)
			*p = '/';
	}

	return 0;
}

Cheers,

-- Slavi
diff mbox series

Patch

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++);
+
+		*p = '\0';
+		ret = mkdir(buf, mode);
+		if (ret < 0) {
+			if (errno != EEXIST) {
+				ret = -errno;
+				break;
+			}
+			ret = 0;
+		}
+		*p = '/';
+	}
+
+	free(buf);
+	return ret;
+}
+
+static int make_fifo(const char *path, mode_t mode)
+{
+	struct stat st;
+	int ret;
+
+	ret = stat(path, &st);
+	if (ret == 0) {
+		if (S_ISFIFO(st.st_mode))
+			return 0;
+		return -EEXIST;
+	}
+
+	return mkfifo(path, mode);
+}
+
+static int make_guest_dir(const char *guest)
+{
+	char path[PATH_MAX];
+
+	snprintf(path, sizeof(path), GUEST_DIR_FMT, guest);
+	return make_dir(path, 0750);
+}
+
+static int make_guest_fifo(const char *guest, int cpu, mode_t mode)
+{
+	static const char *exts[] = {".in", ".out"};
+	char path[PATH_MAX];
+	int i, ret = 0;
+
+	for (i = 0; i < ARRAY_SIZE(exts); i++) {
+		snprintf(path, sizeof(path), GUEST_FIFO_FMT "%s",
+			 guest, cpu, exts[i]);
+		ret = make_fifo(path, mode);
+		if (ret < 0)
+			break;
+	}
+
+	return ret;
+}
+
+static int make_guest_fifos(const char *guest, int nr_cpus, mode_t mode)
+{
+	int i, ret = 0;
+	mode_t mask;
+
+	mask = umask(0);
+	for (i = 0; i < nr_cpus; i++) {
+		ret = make_guest_fifo(guest, i, mode);
+		if (ret < 0)
+			break;
+	}
+	umask(mask);
+
+	return ret;
+}
+
+static void do_setup_guest(const char *guest, int nr_cpus, mode_t mode, gid_t gid)
+{
+	gid_t save_egid;
+	int ret;
+
+	if (gid != -1) {
+		save_egid = getegid();
+		ret = setegid(gid);
+		if (ret < 0)
+			pdie("failed to set effective group ID");
+	}
+
+	ret = make_guest_dir(guest);
+	if (ret < 0)
+		pdie("failed to create guest directory for %s", guest);
+
+	ret = make_guest_fifos(guest, nr_cpus, mode);
+	if (ret < 0)
+		pdie("failed to create FIFOs for %s", guest);
+
+	if (gid != -1) {
+		ret = setegid(save_egid);
+		if (ret < 0)
+			pdie("failed to restore effective group ID");
+	}
+}
+
+void trace_setup_guest(int argc, char **argv)
+{
+	struct group *group;
+	mode_t mode = 0660;
+	int nr_cpus = -1;
+	gid_t gid = -1;
+	char *guest;
+
+	if (argc < 2)
+		usage(argv);
+
+	if (strcmp(argv[1], "setup-guest") != 0)
+		usage(argv);
+
+	for (;;) {
+		int c, option_index = 0;
+		static struct option long_options[] = {
+			{"help", no_argument, NULL, '?'},
+			{NULL, 0, NULL, 0}
+		};
+
+		c = getopt_long(argc-1, argv+1, "+hc:p:g:",
+				long_options, &option_index);
+		if (c == -1)
+			break;
+		switch (c) {
+		case 'h':
+			usage(argv);
+			break;
+		case 'c':
+			nr_cpus = atoi(optarg);
+			break;
+		case 'p':
+			mode = strtol(optarg, NULL, 8);
+			break;
+		case 'g':
+			group = getgrnam(optarg);
+			if (!group)
+				pdie("group %s does not exist", optarg);
+			gid = group->gr_gid;
+			break;
+		default:
+			usage(argv);
+		}
+	}
+
+	if (optind != argc-2)
+		usage(argv);
+
+	guest = argv[optind+1];
+
+	if (nr_cpus <= 0)
+		pdie("invalid number of cpus for guest %s", guest);
+
+	do_setup_guest(guest, nr_cpus, mode, gid);
+}
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index e845f50..7a8002f 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -246,6 +246,14 @@  static struct usage_help usage_help[] = {
 		"          -p port number to listen on.\n"
 		"          -D run in daemon mode.\n"
 	},
+	{
+		"setup-guest",
+		"create FIFOs for tracing guest VMs",
+		" %s setup-guest -c cpus[-p perm][-g group] guest\n"
+		"          -c number of guest virtual CPUs\n"
+		"          -p FIFOs permissions (default: 0660)\n"
+		"          -g FIFOs group owner\n"
+	},
 #endif
 	{
 		"list",