mkdir -p $runstatedir at run time, not install time

[alejandro-colomar]

Not tested. I just built it.

Closes: https://github.com/nginx/unit/issues/742 Fixes: 5a37171f733fa8c7326161cc49af3df0be5dfae4 ("Added default values for pathnames.") Fixes: 57fc9201cb91e3d8901a64e3daaaf31684ee5bf5 ("Socket: Created control socket & pid file directories.") Reported-by: @andypost Cc: @ac000 Cc: @thresheek Cc: @lcrilly

[alejandro-colomar]

v1b changes:

• Copyright.

$ git range-diff ngx/master gh/mkdir mkdir 
1:  27cfc893 = 1:  27cfc893 Fs: Invert logic to reduce indentation
2:  1091f158 ! 2:  3cf22abb Fs: Correctly handle "/" in nxt_fs_mkdir_parent()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
    +@@
    + /*
    +  * Copyright (C) NGINX, Inc.
    ++ * Copyright 2024, Alejandro Colomar <alx@kernel.org>
    +  */
    + 
    + #include <nxt_main.h>
     @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
          ret = NXT_OK;

3:  3bd1d9c2 = 3:  4d681b0b Fs: Make parents recursively in nxt_fs_mkdir_parent()
4:  81d5133c = 4:  3889f2cb Auto: Don't install $runstatedir

[alejandro-colomar]

v2 changes:

• Use correct variable (path => dir) (Whoops)
• Ignore EEXIST, as mkdir -p does. Otherwise, it'll fail creating some top-level dir very likely.

$ git range-diff --creation-factor=80 ngx/master gh/mkdir mkdir 
1:  27cfc893 = 1:  27cfc893 Fs: Invert logic to reduce indentation
2:  3cf22abb = 2:  3cf22abb Fs: Correctly handle "/" in nxt_fs_mkdir_parent()
3:  4d681b0b ! 3:  0bf3d2ac Fs: Make parents recursively in nxt_fs_mkdir_parent()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
    +@@
    + 
    + #include <nxt_main.h>
    + 
    ++#include <errno.h>
    ++
    + 
    + static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);
    + 
     @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
          }

          *ptr = '\0';
     +    if (strcmp(dir, (const char *) path) != 0)
     +    {
    -+        ret = nxt_fs_mkdir_parent(path, mode);
    ++        ret = nxt_fs_mkdir_parent((const u_char *) dir, mode);
     +        if (nxt_slow_path(ret == NXT_ERROR)) {
     +            goto out_free;
     +        }
     +    }
    ++
          ret = nxt_fs_mkdir((const u_char *) dir, mode);
    ++    if (ret == NXT_ERROR && errno == EEXIST) {
    ++        ret = NXT_OK;
    ++    }

      out_free:
    +     nxt_free(dir);
4:  3889f2cb = 4:  b2e61d0e Auto: Don't install $runstatedir

Now it's lightly tested, but only lightly.

[alejandro-colomar]

v3 changes:

• ptr == dir is not a slow path when creating parents recursively. It will happen when arriving at "/".

$ git range-diff ngx/master gh/mkdir mkdir 
1:  27cfc893 = 1:  27cfc893 Fs: Invert logic to reduce indentation
2:  3cf22abb = 2:  3cf22abb Fs: Correctly handle "/" in nxt_fs_mkdir_parent()
3:  0bf3d2ac ! 3:  a3654f39 Fs: Make parents recursively in nxt_fs_mkdir_parent()
    @@ src/nxt_fs.c
      static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);

     @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
    +     ret = NXT_OK;
    + 
    +     ptr = strrchr(dir, '/');
    +-    if (nxt_slow_path(ptr == NULL || ptr == dir)) {
    ++    if (ptr == dir || nxt_slow_path(ptr == NULL)) {
    +         goto out_free;
          }

          *ptr = '\0';
4:  b2e61d0e = 4:  c552be75 Auto: Don't install $runstatedir

[ac000]

IIRC the initial intention was to create the full paths, but there was some reason not to... can't remember where that discussion took place...

[alejandro-colomar]

IIRC the initial intention was to create the full paths,

Yup. IIRC, we checked that other programs behaved like mkdir -p too, so there's prior art.

but there was some reason not to... can't remember where that discussion took place...

Yeah, that was @thresheek . I don't remember where we talked about it, but I remember what, +/-. He was concerned about mkdir -p being too much, and keeping it simpler if not necessary, to reduce chances of doing something wrong.

Well, I'd say we need it, so unless he has stronger reasons to avoid it, I'd go for it. I CCed him, in case he wants to speak.

[alejandro-colomar]

v4 changes:

• Extend commit message for 1.
• Rename function that does mkdir -p $(dirname ...) to the obvious name.

$ git range-diff --creation-factor=80 master gh/mkdir mkdir 
1:  27cfc893 ! 1:  fd8279d0 Fs: Invert logic to reduce indentation
    @@ Metadata
      ## Commit message ##
         Fs: Invert logic to reduce indentation

    +    This refactor isn't very appealing alone, but it prepares the code for
    +    the following commits.
    +
         Link: <https://github.com/nginx/unit/issues/742>
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Liam Crilly <liam@nginx.com>
2:  3cf22abb = 2:  1f20427e Fs: Correctly handle "/" in nxt_fs_mkdir_parent()
3:  a3654f39 ! 3:  4ed9df7c Fs: Make parents recursively in nxt_fs_mkdir_parent()
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    Fs: Make parents recursively in nxt_fs_mkdir_parent()
    +    Fs: Make parents recursively for the pid file and the control socket

         Build systems should not attempt to create $runstatedir (or anything
         under it).  Doing so causes warnings in packaging systems, such as in
    @@ Commit message

         But unitd(8) can be configured to be installed under /opt, or other
         trees, where no directories exist before hand.  Expecting that the user
    -    creates the entire directory trees that unit will expect is a bit
    +    creates the entire directory trees that unit will need is a bit
         unreasonable.  Instead, we can just create any directories that we need,
         with all their parents.

    @@ Commit message
         Cc: Konstantin Pavlov <thresh@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

    + ## src/nxt_controller.c ##
    +@@ src/nxt_controller.c: nxt_runtime_controller_socket(nxt_task_t *task, nxt_runtime_t *rt)
    +     if (ls->sockaddr->u.sockaddr.sa_family == AF_UNIX) {
    +         const char *path = ls->sockaddr->u.sockaddr_un.sun_path;
    + 
    +-        nxt_fs_mkdir_parent((const u_char *) path, 0755);
    ++        nxt_fs_mkdir_parents_dirname((const u_char *) path, 0755);
    +     }
    + #endif
    + 
    +
      ## src/nxt_fs.c ##
     @@

    @@ src/nxt_fs.c

      static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);

    +@@ src/nxt_fs.c: nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    + 
    + 
    + nxt_int_t
    +-nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
    ++nxt_fs_mkdir_parents_dirname(const u_char *path, mode_t mode)
    + {
    +     char       *ptr, *dir;
    +     nxt_int_t  ret;
     @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
          ret = NXT_OK;

    @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)
          *ptr = '\0';
     +    if (strcmp(dir, (const char *) path) != 0)
     +    {
    -+        ret = nxt_fs_mkdir_parent((const u_char *) dir, mode);
    ++        ret = nxt_fs_mkdir_parents_dirname((const u_char *) dir, mode);
     +        if (nxt_slow_path(ret == NXT_ERROR)) {
     +            goto out_free;
     +        }
    @@ src/nxt_fs.c: nxt_fs_mkdir_parent(const u_char *path, mode_t mode)

      out_free:
          nxt_free(dir);
    +
    + ## src/nxt_fs.h ##
    +@@
    + #define _NXT_FS_H_INCLUDED_
    + 
    + 
    +-nxt_int_t nxt_fs_mkdir_parent(const u_char *path, mode_t mode);
    ++nxt_int_t nxt_fs_mkdir_parents_dirname(const u_char *path, mode_t mode);
    + nxt_int_t nxt_fs_mkdir_all(const u_char *dir, mode_t mode);
    + 
    + 
    +
    + ## src/nxt_runtime.c ##
    +@@ src/nxt_runtime.c: nxt_runtime_pid_file_create(nxt_task_t *task, nxt_file_name_t *pid_file)
    + 
    +     file.name = pid_file;
    + 
    +-    nxt_fs_mkdir_parent(pid_file, 0755);
    ++    nxt_fs_mkdir_parents_dirname(pid_file, 0755);
    + 
    +     n = nxt_file_open(task, &file, O_WRONLY, O_CREAT | O_TRUNC,
    +                       NXT_FILE_DEFAULT_ACCESS);
4:  c552be75 = 4:  bb26587a Auto: Don't install $runstatedir

[alejandro-colomar]

nxt_fs_mkdir_all

Now that I see, there's nxt_fs_mkdir_all(). I guess it's mkdir -p, so maybe I can just call that internally. And we can rename it to _mkdir_parents, to match mkdir -p.

[alejandro-colomar]

nxt_fs_mkdir_all

Now that I see, there's _nxt_fs_mkdirall(). I guess it's mkdir -p, so maybe I can just call that internally. And we can rename it to _mkdir_parents, to match mkdir -p.

However, I see that function asserts that dir[0] == '/', which need not be true in the case of the pid and control socket. I remember @lcrilly configures with --prefix=./build/ or something like that, for running from the build tree.

How much is that assertion necessary for the code that uses this function? Can we just remove it?

[ac000]

Why can't we just use nxt_fs_mkdir_all()? (now that I remember it exists!)

We basically just want to do mkdir -p isn't it?

Which I'm sure is what I would have done in the first place, except we didn't for $reason, and I had to create nxt_fs_mkdir_parent() to only create the last directory...

[alejandro-colomar]

Why can't we just use nxt_fs_mkdir_all()? (now that I remember it exists!)

I'm adapting it to be able to use it. Out-of-the-box, it has an assertion that makes me suspect it won't please @lcrilly , who runs unitd(8) with a relative $prefix.

However, I'm trying to adapt the function to be able to use it.

We basically just want to do mkdir -p isn't it?

Actually, mkdir -p $(dirname ...), but yeah, mostly.

Which I'm sure is what I would have done in the first place,

Yup. We were almost going to do that, IIRC, when @thresheek suggested not doing so. Out of caution, we didn't. :)

except we didn't for $reason, and I had to create nxt_fs_mkdir_parent() to only create the last directory...

I wish I had suggested calling that _dirname instead of _parent back then. :)

(And call the other one _parents instead of _all.)

[ac000]

I think I'm still missing something, why doesn't nxt_fs_mkdir_all() work for this use case? (forget about corner cases for now and forget all about nxt_fs_mkdir_parent())

It works for the cgroups and isolation/rootfs cases where they need to create full directory paths...

In src/nxt_isolation.c::nxt_isolation_prepare_rootfs()

(src/nxt_isolation.c:783: ret = nxt_fs_mkdir_all(dst, S_IRWXU | S_IRWXG | S_IRWXO);)

ret = nxt_fs_mkdir_all(dst, S_IRWXU | S_IRWXG | S_IRWXO);               

which for example does

[pid 65887] mkdir("/srv", 0777)         = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit", 0777)    = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test", 0777) = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test/usr", 0777) = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test/usr/lib", 0777) = 0
[pid 65887] mkdir("/srv/unit/test/usr/lib/python3.12", 0777) = 0
[pid 65887] mkdir("/srv/unit/test/usr/lib/python3.12/site-packages", 0777) = 0

[ac000]

I mean shouldn't you bascially just be able to replace calls to nxt_fs_mkdir_parent() with nxt_fs_mkdir_all()?

[alejandro-colomar]

I think I'm still missing something, why doesn't nxt_fs_mkdir_all() work for this use case? (forget about corner cases for now and forget all about nxt_fs_mkdir_parent())

It works for the cgroups and isolation/rootfs cases where they need to create full directory paths...

In src/nxt_isolation.c::nxt_isolation_prepare_rootfs()

(src/nxt_isolation.c:783: ret = nxt_fs_mkdir_all(dst, S_IRWXU | S_IRWXG | S_IRWXO);)

ret = nxt_fs_mkdir_all(dst, S_IRWXU | S_IRWXG | S_IRWXO);               

which for example does

[pid 65887] mkdir("/srv", 0777)         = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit", 0777)    = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test", 0777) = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test/usr", 0777) = -1 EEXIST (File exists)
[pid 65887] mkdir("/srv/unit/test/usr/lib", 0777) = 0
[pid 65887] mkdir("/srv/unit/test/usr/lib/python3.12", 0777) = 0
[pid 65887] mkdir("/srv/unit/test/usr/lib/python3.12/site-packages", 0777) = 0

We don't want to mkdir the last path-name segment. The last path-name segment is the socket or the pid file.

[alejandro-colomar]

I mean shouldn't you bascially just be able to replace calls to nxt_fs_mkdir_parent() with nxt_fs_mkdir_all()?

Yes, I should be able to do that (to replace the recursion). In fact, that's what I want to do, now that we remember it exists. :-)

We still need the wrapper call to get the dirname.

[ac000]

We don't want to mkdir the last path-name segment. The last path-name segment is the socket or the pid file.

Oh, right, bugger...

Hmm, I'm not convinced trying to mould nxt_fs_mkdir_parent() is the right thing to do here...

I would probably just create a simple helper function that uses dirname(3) to get rid of the file part and then call's nxt_fs_mkdir_all()

[ac000]

But maybe we should wait for further feedback if this is even the way we should go...

[alejandro-colomar]

We don't want to mkdir the last path-name segment. The last path-name segment is the socket or the pid file.

Oh, right, bugger...

Hmm, I'm not convinced trying to mould nxt_fs_mkdir_parent() is the right thing to do here...

I would probably just create a simple helper function that uses dirname(3) to get rid of the file part and then call's nxt_fs_mkdir_all()

That's my plan. I'm going to drop this patch, and rewrite it to wrap nxt_fs_mkdir_all() in a new function. Which is why I'm first doing some refactors to better understand that code.

[ac000]

If we were to allow relative paths, you'd need to be careful what the current working directory is before hand...

I suppose that's sort of true just as it stands now, though this perhaps works through happenstance more than anything...

$ pwd
/tmp/unit
$ ls -ld build
ls: cannot access 'build': No such file or directory
$ ./configure --prefix=./build/
$ make -j
...
$ strace -f -e mkdirat,mkdir ./build/sbin/unitd --no-daemon
mkdir("./build//var/lib/unit/certs/", 0700) = 0
mkdir("./build//var/lib/unit/scripts/", 0700) = 0
mkdir("./build//var/run/unit", 0755)    = -1 EEXIST (File exists)
mkdir("./build//var/run/unit", 0755)    = -1 EEXIST (File exists)
$ cd ..
$ pwd
/tmp
$ strace -f -e mkdirat,mkdir ./unit/build/sbin/unitd --no-daemon
mkdir("./build//var/lib/unit/certs/", 0700) = -1 ENOENT (No such file or directory)
mkdir("./build//var/lib/unit/scripts/", 0700) = -1 ENOENT (No such file or directory)
mkdir("./build//var/run/unit", 0755)    = -1 ENOENT (No such file or directory)

But it is a use case we should keep working...

[alejandro-colomar]

I'm closing, in favour of https://github.com/nginx/unit/pull/1235.

[alejandro-colomar]

If we were to allow relative paths, you'd need to be careful what the current working directory is before hand...

I suppose that's sort of true just as it stands now, though this perhaps works through happenstance more than anything...

@lcrilly and I worked on making that work. It's a bit brittle, in that you must run unitd(8) from the right working directory, but it works. That's just for running from the source repository, when in a edit-compile-test loop, without having to install (I do install under /opt for those things, but Liam prefers running from there directly.) So, users of this feature should know what they're doing.

$ pwd
/tmp/unit
$ ls -ld build
ls: cannot access 'build': No such file or directory
$ ./configure --prefix=./build/
$ make -j
...
$ strace -f -e mkdirat,mkdir ./build/sbin/unitd --no-daemon
mkdir("./build//var/lib/unit/certs/", 0700) = 0
mkdir("./build//var/lib/unit/scripts/", 0700) = 0
mkdir("./build//var/run/unit", 0755)    = -1 EEXIST (File exists)
mkdir("./build//var/run/unit", 0755)    = -1 EEXIST (File exists)
$ cd ..
$ pwd
/tmp
$ strace -f -e mkdirat,mkdir ./unit/build/sbin/unitd --no-daemon
mkdir("./build//var/lib/unit/certs/", 0700) = -1 ENOENT (No such file or directory)
mkdir("./build//var/lib/unit/scripts/", 0700) = -1 ENOENT (No such file or directory)
mkdir("./build//var/run/unit", 0755)    = -1 ENOENT (No such file or directory)

But it is a use case we should keep working...