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