Skip to content
This repository was archived by the owner on Oct 8, 2025. It is now read-only.

Conversation

@alejandro-colomar
Copy link
Contributor

@alejandro-colomar alejandro-colomar commented Apr 22, 2024

Not tested. I just built it.

Closes: #742
Fixes: 5a37171 ("Added default values for pathnames.")
Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Reported-by: @andypost
Cc: @ac000
Cc: @thresheek
Cc: @lcrilly

@alejandro-colomar
Copy link
Contributor Author

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
Copy link
Contributor Author

alejandro-colomar commented Apr 22, 2024

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
Copy link
Contributor Author

alejandro-colomar commented Apr 22, 2024

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
Copy link
Member

ac000 commented Apr 22, 2024

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
Copy link
Contributor Author

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.

Comment on lines 65 to 87
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';
if (strcmp(dir, (const char *) path) != 0)
{
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you just create 'mkdir -p'?

Maybe the function should be renamed more appropriately (assuming it doesn't need to be retained in its original form)

Also not terribly fond of the recursion... or if some nutter tries to create /a/b/c/d/e/f/g/h/i/j/..., but I can probably live with it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought of looping, but since this is something we run just twice, and only at program startup, I guess it's simpler to recurse.

Regarding the name... I thought originally of naming it _parents instead of _parent. However, I thought _parents might be confusing, as it could lead to think that it's equivalent to mkdir -p, while it's equivalent to mkdir -p $(dirname ...). Now that I write this, the obvious name comes to my head as nxt_fs_mkdir_dirname_parents(). Does that sound good to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe _mkdir_parents_dirname ...

src/nxt_fs.c Outdated

ptr = strrchr(dir, '/');
if (nxt_slow_path(ptr == NULL)) {
if (nxt_slow_path(ptr == NULL || ptr == dir)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean if someone tried to create the socket or whatever in the root directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. That's unlikely. However, it's necessary for using recursion, since at some point it'll arrive at the parent-most dir, which is of course /usr, and when trying to create its dirname, it'll try /.

This refactor isn't very appealing alone, but it prepares the code for
the following commits.

Link: <nginx#742>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Cc: Andy Postnikov <apostnikov@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The previous code attempted to mkdir(""), that is an empty string.
Since "/" necessarily exists, just goto out_free.

Link: <nginx#742>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Cc: Andy Postnikov <apostnikov@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Build systems should not attempt to create $runstatedir (or anything
under it).  Doing so causes warnings in packaging systems, such as in
Alpine Linux, as reported by Andy.

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 need is a bit
unreasonable.  Instead, we can just create any directories that we need,
with all their parents.

Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Link: <nginx#742>
Reported-by: Andy Postnikov <apostnikov@gmail.com>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This directory should exist already in the system, and if not, it should
(and will) be created at run time, not at install time.

It triggered a warning in Alpine Linux's packaging system:

    ERROR: unit*: Packages must not put anything under /var/run

Fixes: 5a37171 ("Added default values for pathnames.")
Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Closes: <nginx#742>
Reported-by: Andy Postnikov <apostnikov@gmail.com>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Contributor Author

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
Copy link
Contributor Author

alejandro-colomar commented Apr 23, 2024

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
Copy link
Contributor Author

alejandro-colomar commented Apr 23, 2024

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.

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
Copy link
Member

ac000 commented Apr 23, 2024

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
Copy link
Contributor Author

alejandro-colomar commented Apr 23, 2024

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
Copy link
Member

ac000 commented Apr 23, 2024

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
Copy link
Member

ac000 commented Apr 23, 2024

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

@alejandro-colomar
Copy link
Contributor Author

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
Copy link
Contributor Author

alejandro-colomar commented Apr 23, 2024

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
Copy link
Member

ac000 commented Apr 23, 2024

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
Copy link
Member

ac000 commented Apr 23, 2024

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

@alejandro-colomar
Copy link
Contributor Author

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.

@alejandro-colomar alejandro-colomar marked this pull request as draft April 23, 2024 23:32
@ac000
Copy link
Member

ac000 commented Apr 24, 2024

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
Copy link
Contributor Author

I'm closing, in favour of #1235.

@alejandro-colomar
Copy link
Contributor Author

alejandro-colomar commented Apr 26, 2024

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The $runstatedir is not being created at runtime

2 participants