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

[Steven Rostedt <rostedt@goodmis.org>]

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