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

[Slavomir Kaslev <kaslevs@vmware.com>]

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