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