On Mon, 18 Feb 2019 16:37:47 +0200
Slavomir Kaslev <kaslevs@xxxxxxxxxx> 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
|