fs: Rename functions

[alejandro-colomar]

Bogus ruby test.

[ac000]

To be brutally honest I don't see the point of either of these...

In terms of the name, dirname means strip off the last directory component, not what nxt_fs_mkdir_parent() does (it creates the last directory component).

As for the other, I know the -p in mkdir is --parents, but I think all is also perfectly descriptive... (anyway _parents is too verbose, personally I'd have gone for _p).

[ac000]

Damn, why does this ruby test keep failing for you?! I only see it occasionally fail...

@andrey-zelenkov @arbourd any ideas?

[alejandro-colomar]

To be brutally honest I don't see the point of either of these...

I wouldn't mind for the code as is, but I'm going to change nxt_fs_mkdir_parent() to be nxt_fs_mkdir_p_dirname() in a future PR, since I want to create all the parents of the dirname (and the dirname itself). This is a preparation to have a consistent naming scheme.

In terms of the name, dirname means strip off the last directory component, not what nxt_fs_mkdir_parent() does (it creates the last directory component).

It strips off the last path-name component, not directory component. Usually this is done to strip a regular file name, and refer to the (parent) directory that contains it (thus the name).

$ dirname dir1/dir2/file
dir1/dir2

nxt_fs_mkdir_parent() creates precisely the dirname of the path that you pass to it.

alx@debian:~/src/nginx/unit/master$ grepc nxt_fs_mkdir_parent .
./src/nxt_fs.c:nxt_int_t
nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
{
    ...

    dir = nxt_strdup(path);
    ...

    ptr = strrchr(dir, '/');
    if (nxt_fast_path(ptr != NULL)) {
        *ptr = '\0';
        ret = nxt_fs_mkdir((const u_char *) dir, mode);
    }

    ...
}

nxt_fs_mkdir_parent("dir1/dir2/file", mode);

is literally equivalent (ignoring the mode) to

mkdir $(dirname "dir1/dir2/file")

which evaluates to

mkdir dir1/dir2

As for the other, I know the -p in mkdir is --parents, but I think all is also perfectly descriptive... (anyway _parents is too verbose, personally I'd have gone for _p).

Hmmm, I like _p.

[alejandro-colomar]

v2 changes:

• shorten name: _parents => _p [@ac000 ]

$ git range-diff master gh/fname fname 
1:  7a542e2c = 1:  7a542e2c fs: Rename function
2:  49b59aaf ! 2:  29e3efb1 fs: Rename function
    @@ src/nxt_cgroup.c: nxt_cgroup_proc_add(nxt_task_t *task, nxt_process_t *process)
          }

     -    ret = nxt_fs_mkdir_all((const u_char *) cgprocs, 0777);
    -+    ret = nxt_fs_mkdir_parents((const u_char *) cgprocs, 0777);
    ++    ret = nxt_fs_mkdir_p((const u_char *) cgprocs, 0777);
          if (nxt_slow_path(ret == NXT_ERROR)) {
              return NXT_ERROR;
          }
    @@ src/nxt_fs.c: static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);

      nxt_int_t
     -nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    -+nxt_fs_mkdir_parents(const u_char *dir, mode_t mode)
    ++nxt_fs_mkdir_p(const u_char *dir, mode_t mode)
      {
          char    *start, *end, *dst;
          size_t  dirlen;
    @@ src/nxt_fs.h

      nxt_int_t nxt_fs_mkdir_dirname(const u_char *path, mode_t mode);
     -nxt_int_t nxt_fs_mkdir_all(const u_char *dir, mode_t mode);
    -+nxt_int_t nxt_fs_mkdir_parents(const u_char *dir, mode_t mode);
    ++nxt_int_t nxt_fs_mkdir_p(const u_char *dir, mode_t mode);

      #endif  /* _NXT_FS_H_INCLUDED_ */
    @@ src/nxt_isolation.c: nxt_isolation_prepare_rootfs(nxt_task_t *task, nxt_process_
              }

     -        ret = nxt_fs_mkdir_all(dst, S_IRWXU | S_IRWXG | S_IRWXO);
    -+        ret = nxt_fs_mkdir_parents(dst, S_IRWXU | S_IRWXG | S_IRWXO);
    ++        ret = nxt_fs_mkdir_p(dst, S_IRWXU | S_IRWXG | S_IRWXO);
              if (nxt_slow_path(ret != NXT_OK)) {
                  nxt_alert(task, "mkdir(%s) %E", dst, nxt_errno);
                  goto undo;

[alejandro-colomar]

So my plan is to end up with a function _mkdir_p() that does precisely mkdir -p $path, and a _mkdir_p_dirname() function that does mkdir -p $(dirname $path)

[arbourd]

@ac000 I don't. My cursory understanding is that it is an isolation test that flakes.

SystemExit: Could not unmount filesystems in tmpdir ({temporary_dir}).

It's consistent with Ruby 3.2 though, and I have not seen it with 3.3.

[ac000]

@ac000 I don't. My cursory understanding is that it is an isolation test that flakes.

Correct.

SystemExit: Could not unmount filesystems in tmpdir ({temporary_dir}).

It's consistent with Ruby 3.2 though, and I have not seen it with 3.3.

It always seems to be this one, I see it fail occasionally and it always succeeds if you manually re-run it.

Not sure why it seems to be failing consistently for Alex, but if you manually re-run it it should succeed (it did before).

[ac000]

In my mind they are doing different things.

_parent() may create the directory, but only if all the proceeding directories exist.

But I can see where you're coming from in that _parent() effectively does a dirname(3)...

[ac000]

... and a _mkdir_p_dirname() function that does mkdir -p $(dirname $path)

But that should perhaps not involve nxt_fs_mkdir_parent() at all, as it will end up with different semantics. Creating all the directories vs only the last one.

It may be that _parent() can be dropped, undecided...

[ac000]

So to clarify. Personally I'm OK with _p() and I think _p_dirname() (although a bit unwieldy) should just be a new function.

We can then decide what to do with _parent() afterwards...

(This is all assuming someone doesn't come up with a reason not do this in the first place, like last time)

[alejandro-colomar]

So to clarify. Personally I'm OK with _p() and I think _p_dirname() (although a bit unwieldy) should just be a new function.

Yep.

We can then decide what to do with _parent() afterwards...

My intention was to first rename _parent => _dirname, and later add a commit that drops _dirname and adds _p_dirname. Since the code for _p_dirname will be almost the same as for _dirname (because the main job is getting the dirname), it will show as a modification of the function in the commit in git.

[alejandro-colomar]

If you want to see my full intentions, I can write a PR with all the commits together.

[ac000]

My point is we may either want to keep _parent() with its current semantics or drop it... which is why I suggest not to touch it...

[ac000]

If we leave _parent() as is and do remove it, then if at a later date we decide we do want it back, it's a simple revert...

[alejandro-colomar]

If we leave _parent() as is and do remove it, then if at a later date we decide we do want it back, it's a simple revert...

• Please check https://github.com/nginx/unit/pull/1235 and you'll find out why I did what I did. :-)

[alejandro-colomar]

Closing in favour of https://github.com/nginx/unit/pull/1235