mkdir -p $runstatedir at run time, not install time (round 2)

alejandro-colomar commented 2 months ago

Here are all the patches I prepared, in a single PR, tu allow you to understand the global idea I have in mind.

Feel free to apply this branch, or apply refactoring patches separately.

Cc: @ac000 , @andypost, @lcrilly , @thresheek

Supersedes: https://github.com/nginx/unit/pull/1227 Closes: https://github.com/nginx/unit/issues/742

alejandro-colomar commented 2 months ago

Not tested.

andypost commented 2 months ago

Thank you! getting to test

meantime ag shows that defaults still point to /var/run/unit

auto/help
26:  --runstatedir=DIR    default: "\$localstatedir/run/unit"
35:  --pid=FILE           set pid filename, default: "\$runstatedir/unit.pid"
39:                       default: "unix:\$runstatedir/control.unit.sock"

auto/options
84:        --runstatedir=*)                 NXT_RUNSTATEDIR="$value"            ;;
168:NXT_RUNSTATEDIR="${NXT_RUNSTATEDIR-"$NXT_LOCALSTATEDIR/run/unit"}"
169:NXT_CONTROL="${NXT_CONTROL-"unix:$NXT_RUNSTATEDIR/control.unit.sock"}"
170:NXT_PID="${NXT_PID-"$NXT_RUNSTATEDIR/unit.pid"}"

alejandro-colomar commented 2 months ago

Thank you! getting to test

meantime ag shows that defaults still point to /var/run/unit

The default is actually /usr/local/var/run/unit. See:

$ ./configure --help | grep -e =DIR -e =FILE -e =ADDRESS
  --cc=FILE            set C compiler filename, default: "cc"
  --prefix=DIR         default: "/usr/local"
  --exec-prefix=DIR    default: "$prefix"
  --bindir=DIR         default: "$exec_prefix/bin"
  --sbindir=DIR        default: "$exec_prefix/sbin"
  --includedir=DIR     default: "$prefix/include"
  --libdir=DIR         default: "$exec_prefix/lib"
  --modulesdir=DIR     default: "$libdir/unit/modules"
  --datarootdir=DIR    default: "$prefix/share"
  --mandir=DIR         default: "$datarootdir/man"
  --pkgconfigdir=DIR   default: "$datarootdir/pkgconfig"
  --localstatedir=DIR  default: "$prefix/var"
  --statedir=DIR       default: "$localstatedir/lib/unit"
  --runstatedir=DIR    default: "$localstatedir/run/unit"
  --logdir=DIR         default: "$localstatedir/log/unit"
  --tmpdir=DIR         default: "/tmp"
  --incdir=DIR         [deprecated] synonym for --includedir
  --modules=DIR        [deprecated] synonym for --modulesdir
  --state=DIR          [deprecated] synonym for --statedir
  --tmp=DIR            [deprecated] synonym for --tmpdir
  --pid=FILE           set pid filename, default: "$runstatedir/unit.pid"
  --log=FILE           set log filename, default: "$logdir/unit.log"
  --control=ADDRESS    set address of control API socket

But yeah, assuming that you specify --localstatedir=/var, you'll end up with $runstatedir being /var/run/unit.

Regarding the FHS 3.0, I discussed about it with an OpenBSD maintainer, and he criticized the FHS as something Linux-centric that didn't take into account the BSDs. As a consequence, the BSDs don't consider the FHS as a valid standard; it's still a good reference, but in this specific case, the BSDs disagree, and so we need to use a value that will work in all systems.

In your case, I suggest specifying --runstatedir to your favourite value, likely /run/unit, I guess.

Or do you mean that even if you specify that some things are hardcoded?

alejandro-colomar commented 2 months ago

v2 changes:

Consistently use octal mode.

$ git range-diff master gh/fs fs 
 1:  a97da9fb =  1:  a97da9fb fs: Use branchless code
 2:  990b238f =  2:  990b238f fs: Use a temporary variable for the return value
 3:  8a72be25 =  3:  8a72be25 fs: Accept relative paths in nxt_fs_mkdir_all()
 4:  2381359d =  4:  2381359d fs: Accept path names of length 1
 5:  1d60a9b3 =  5:  1d60a9b3 fs: Rename function
 6:  f2ce48fb =  6:  f2ce48fb fs: Rename function
 7:  cff46fe8 =  7:  cff46fe8 fs: Invert logic to reduce indentation
 8:  1144b573 =  8:  1144b573 fs: Correctly handle "/" in nxt_fs_mkdir_parent()
 9:  8853946e =  9:  8853946e fs: Make parents recursively for the pid file and the control socket
10:  ec0bdf3a = 10:  ec0bdf3a auto: Don't install $runstatedir
 -:  -------- > 11:  e29e134b Use octal instead of mode macros

andypost commented 2 months ago

Thank you! specified --runstatedir="/run" for configure and looks like all tests passed (sadly armv7 and armhf hanging after packaging in CI but that's preexisting)

EDIT ref https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/64807

andypost commented 2 months ago

Is there any specific need to have /run/unit if it used for socket and pid-file only?

alejandro-colomar commented 2 months ago

Is there any specific need to have /run/unit if it used for socket and pid-file only?

The FHS 3.0 :-)

Programs may have a subdirectory of /run; this is encouraged for programs that use more than one run-time file.

We have two run-time files, which is "more than one run-time file".

alejandro-colomar commented 2 months ago

BTW, I see in https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/64807/diffs that you specify --control="unix:/run/control.unit.sock" and --pid="/run/unit.pid". You can drop those definitions now that you specify $runstatedir.

You also specify --statedir="/var/lib/unit", and you can also drop it, since you specify $localstatedir:

$ ./configure --help | grep '\<statedir'
  --statedir=DIR       default: "$localstatedir/lib/unit"
  --state=DIR          [deprecated] synonym for --statedir

Unless you want to keep them to be stable even if we change the defaults (hopefully, shouldn't happen, but might).

alejandro-colomar commented 2 months ago

v2b changes:

Add some Fixes: fields

$ git range-diff master gh/fs fs 
 1:  a97da9fb =  1:  a97da9fb fs: Use branchless code
 2:  990b238f =  2:  990b238f fs: Use a temporary variable for the return value
 3:  8a72be25 =  3:  8a72be25 fs: Accept relative paths in nxt_fs_mkdir_all()
 4:  2381359d !  4:  4a63d651 fs: Accept path names of length 1
    @@ Commit message

         That is, accept "/", or relative path names of a single byte.

    +    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 5:  1d60a9b3 =  5:  1b56419a fs: Rename function
 6:  f2ce48fb =  6:  feeea759 fs: Rename function
 7:  cff46fe8 =  7:  c87e2911 fs: Invert logic to reduce indentation
 8:  1144b573 !  8:  41f87976 fs: Correctly handle "/" in nxt_fs_mkdir_parent()
    @@ Commit message
         The previous code attempted to mkdir(""), that is an empty string.
         Since "/" necessarily exists, just goto out_free.

    +    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Link: <https://github.com/nginx/unit/issues/742>
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Liam Crilly <liam@nginx.com>
 9:  8853946e =  9:  e0eaaf06 fs: Make parents recursively for the pid file and the control socket
10:  ec0bdf3a = 10:  3d1b94df auto: Don't install $runstatedir
11:  e29e134b = 11:  09fd8bc8 Use octal instead of mode macros

alejandro-colomar commented 2 months ago

Thank you! specified --runstatedir="/run" for configure and looks like all tests passed (sadly armv7 and armhf hanging after packaging in CI but that's preexisting)

EDIT ref https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/64807

I transformed this comment into a Tested-by: Andy Postnikov <...> field for all the commits (except for the last one; since I added that one later and don't know if you tested it). Please express yourself if you want to modify that.

Thanks!

v2c changes:

Add tested-by field [@andypost ]

$ git range-diff master gh/fs fs 
 1:  a97da9fb !  1:  fd618a14 fs: Use branchless code
    @@ Commit message
         in each iteration.

         Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 2:  990b238f !  2:  e9b3b2fe fs: Use a temporary variable for the return value
    @@ Commit message

         This avoids breaking a long line.

    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 3:  8a72be25 !  3:  472069f8 fs: Accept relative paths in nxt_fs_mkdir_all()
    @@ Metadata
      ## Commit message ##
         fs: Accept relative paths in nxt_fs_mkdir_all()

    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 4:  4a63d651 !  4:  ee134aa1 fs: Accept path names of length 1
    @@ Commit message
         That is, accept "/", or relative path names of a single byte.

         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 5:  1b56419a !  5:  4ab479f8 fs: Rename function
    @@ Commit message
         to mkdir -p, which is too similar to "parent", so it can lead to
         confusion.

    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_controller.c ##
 6:  feeea759 !  6:  3d2a6f3c fs: Rename function
    @@ Commit message
         of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
         should be more straightforward to readers.

    +    Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_cgroup.c ##
 7:  c87e2911 !  7:  b2706fed fs: Invert logic to reduce indentation
    @@ Commit message
         the following commits.

         Link: <https://github.com/nginx/unit/issues/742>
    +    Tested-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>
    -    Cc: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 8:  41f87976 !  8:  5d3a1fd4 fs: Correctly handle "/" in nxt_fs_mkdir_parent()
    @@ Commit message

         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Link: <https://github.com/nginx/unit/issues/742>
    +    Tested-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>
    -    Cc: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 9:  e0eaaf06 !  9:  1feec376 fs: Make parents recursively for the pid file and the control socket
    @@ Commit message
         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Link: <https://github.com/nginx/unit/issues/742>
         Reported-by: Andy Postnikov <apostnikov@gmail.com>
    +    Tested-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>
10:  3d1b94df ! 10:  bd1f96fb auto: Don't install $runstatedir
    @@ Commit message
         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Closes: <https://github.com/nginx/unit/issues/742>
         Reported-by: Andy Postnikov <apostnikov@gmail.com>
    +    Tested-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>
11:  09fd8bc8 = 11:  4ab657ad Use octal instead of mode macros

ac000 commented 2 months ago

 fs: Correctly handle "/" in nxt_fs_mkdir_parent()

The previous code attempted to mkdir(""), that is an empty string.
Since "/" necessarily exists, just goto out_free.

Only if they attempted something bonkers like --pid /

alejandro-colomar commented 2 months ago

 fs: Correctly handle "/" in nxt_fs_mkdir_parent()

The previous code attempted to mkdir(""), that is an empty string.
Since "/" necessarily exists, just goto out_free.

Only if they attempted something bonkers like --pid /

Actually, --pid /unit.pid would also result in that, since we get the dirname of it. These days, people often place stuff in the root of docker containers (I don't approve that practice, but people do it). It's unlikely, but it could happen.

alejandro-colomar commented 2 months ago

fs: Make parents recursively for the pid file and the control socket 
I'm not sure recursively is the right term to use here. perhaps
fs: Make the full directory path for the pid file and control socket

Ahh, yeah, I just pasted the commit message from the old implementation. I'll revise that.

ac000 commented 2 months ago

Actually, --pid /unit.pid would also result in that, since we get the dirname of it. These days, people often place stuff in the root of docker containers (I don't approve that practice, but people do it). It's unlikely, but it could happen.

Heh, OK, but equally bonkers!

alejandro-colomar commented 2 months ago

v2d changes:

wfix commit message

$ git range-diff master gh/fs fs 
 1:  fd618a14 =  1:  fd618a14 fs: Use branchless code
 2:  e9b3b2fe =  2:  e9b3b2fe fs: Use a temporary variable for the return value
 3:  472069f8 =  3:  472069f8 fs: Accept relative paths in nxt_fs_mkdir_all()
 4:  ee134aa1 =  4:  ee134aa1 fs: Accept path names of length 1
 5:  4ab479f8 =  5:  4ab479f8 fs: Rename function
 6:  3d2a6f3c =  6:  3d2a6f3c fs: Rename function
 7:  b2706fed =  7:  b2706fed fs: Invert logic to reduce indentation
 8:  5d3a1fd4 =  8:  5d3a1fd4 fs: Correctly handle "/" in nxt_fs_mkdir_parent()
 9:  1feec376 !  9:  5ab148a8 fs: Make parents recursively for the pid file and the control socket
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Make parents recursively for the pid file and the control socket
    +    fs: Make the full directory path 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 need is a bit
    -    unreasonable.  Instead, we can just create any directories that we need,
    -    with all their parents.
    +    unreasonable.  Instead, let's just create any directories that we need,
    +    with all their parents, at run time.

         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Link: <https://github.com/nginx/unit/issues/742>
10:  bd1f96fb = 10:  c8e54495 auto: Don't install $runstatedir
11:  4ab657ad = 11:  66c94bc2 Use octal instead of mode macros

alejandro-colomar commented 2 months ago

On Thu, Apr 25, 2024 at 06:18:47AM -0700, Andrew Clayton wrote:

Perhaps rather than simply 'Rename function' you could specify the function name, i.e
fs: Rename nxt_fs_mkdir_parent()`

Hmm, yeah. How about this?:

fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 2 months ago

On Thu, Apr 25, 2024 at 06:49:43AM -0700, Andrew Clayton wrote:

@ac000 commented on this pull request.
 fs: Use branchless code 
Could we make that
fs: Use branchless code in nxt_fs_mkdir_all()

LGTM

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 2 months ago

On Thu, Apr 25, 2024 at 07:04:02AM -0700, Andrew Clayton wrote:

@ac000 commented on this pull request.

char start, end, *dst;

size_t dirlen;

char path[PATH_MAX];

char start, end, *dst;

size_t dirlen;

nxt_int_t ret;

char path[PATH_MAX];

Personally I'm not fussed about having Christmas tree variable type ordering and it was 'wrong' before, so meh...

The Christmas tree variable ordering is documented here: https://nginx.org/en/docs/dev/development_guide.html#code_style_variables :D

( I could also quite happily live without the alignment, and the multiple variable declaration on a single line for that matter, it would make changes like this simpler)

I like the alignment. I don't love the exception for arrays, though, and yeah, it causes diffs to be larger than they need, but it results in more readable code. Maybe the array exception could be dropped.

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 2 months ago

On Thu, Apr 25, 2024 at 07:06:58AM -0700, Andrew Clayton wrote:

@ac000 commented on this pull request.

Perhaps the subject
 fs: Use a temporary variable for the return value
Could be expanded to
fs: Use a temporary variable for the return value in nxt_fs_mkdir_all()

Indeed; the function name is usually relevant for these things. I lately tend to use two prefixes in my commit messages:

prefix: func: subject

(For the prefix, I use the file name)

So in this case I would have written:

fs: nxt_fs_mkdir_all(): Use a temporary variable for the return value

or even

src/nxt_fs.c: nxt_fs_mkdir_all(): Use a temporary variable for the return value

However, Andrei didn't like having a second prefix, so I guess

... in foo()

is good.

-- https://www.alejandro-colomar.es/

ac000 commented 2 months ago

The prefixes are just supposed to be general topics/area's of the code, so I wouldn't put a function name in there...

ac000 commented 2 months ago

The Christmas tree variable ordering is documented here: https://nginx.org/en/docs/dev/development_guide.html#code_style_variables

Yeah, seen it before, we don't strictly follow that anyway, e.g pointer alignment.

Too many idiosyncrasies...

I like the alignment. I don't love the exception for arrays, though, and yeah, it causes diffs to be larger than they need, but it results in more readable code. Maybe the array exception could be dropped.

Personally I don't have any problem reading them unaligned, it's what I've been most used to over the past decades...

alejandro-colomar commented 2 months ago

The prefixes are just supposed to be general topics/area's of the code, so I wouldn't put a function name in there...

Ok. This time I've used 2 prefixes (I hadn't seen your message). I'll send a next round in a moment, moving it to the English sentence.

v2e changes:

Put the renaming commits first.
Add a second prefix to commit messages whose changes are scoped to a single function.

$ git range-diff master gh/fs fs 
 5:  4ab479f8 !  1:  f497b51f fs: Rename function
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Rename function
    +    fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()

         "dirname" is the usual way to refer to the directory part of a path
         name.  See for example dirname(1), or the dirname builtin in several
 6:  3d2a6f3c !  2:  44e2b3b8 fs: Rename function
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Rename function
    +    fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()

         "all" is too generic of an attribute to be meaningful.  In the context
         of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
    @@ src/nxt_fs.c: static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);
     -nxt_fs_mkdir_all(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;
    +     char    *start, *end, *dst;
    +     size_t  dirlen;

      ## src/nxt_fs.h ##
     @@
 1:  fd618a14 !  3:  02fedfb9 fs: Use branchless code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Use branchless code
    +    fs: nxt_fs_mkdir_p(): Use branchless code

         That branch was to avoid an infinite loop on the slash.  However, we can
         achieve the same by using a +1 to make sure we advance at least 1 byte
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
    -@@ src/nxt_fs.c: nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    +@@ src/nxt_fs.c: nxt_fs_mkdir_p(const u_char *dir, mode_t mode)
          start = (char *) dir;

          while (*start != '\0') {
 2:  e9b3b2fe !  4:  12e79b48 fs: Use a temporary variable for the return value
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Use a temporary variable for the return value
    +    fs: nxt_fs_mkdir_p(): Use a temporary variable

         This avoids breaking a long line.

    @@ Commit message
      ## src/nxt_fs.c ##
     @@ 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_p(const u_char *dir, mode_t mode)
      {
     -    char    *start, *end, *dst;
     -    size_t  dirlen;
    @@ src/nxt_fs.c: static nxt_int_t nxt_fs_mkdir(const u_char *dir, mode_t mode);

          dirlen = nxt_strlen(dir);

    -@@ src/nxt_fs.c: nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    +@@ src/nxt_fs.c: nxt_fs_mkdir_p(const u_char *dir, mode_t mode)
              dst = nxt_cpymem(dst, start, end - start);
              *dst = '\0';

 3:  472069f8 !  5:  8cba8b34 fs: Accept relative paths in nxt_fs_mkdir_all()
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Accept relative paths in nxt_fs_mkdir_all()
    +    fs: nxt_fs_mkdir_p(): Accept relative paths

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
    -@@ src/nxt_fs.c: nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    +@@ src/nxt_fs.c: nxt_fs_mkdir_p(const u_char *dir, mode_t mode)

          dirlen = nxt_strlen(dir);

 4:  ee134aa1 !  6:  49867886 fs: Accept path names of length 1
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Accept path names of length 1
    +    fs: nxt_fs_mkdir_p(): Accept path names of length 1

         That is, accept "/", or relative path names of a single byte.

    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
    -@@ src/nxt_fs.c: nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
    +@@ src/nxt_fs.c: nxt_fs_mkdir_p(const u_char *dir, mode_t mode)

          dirlen = nxt_strlen(dir);

 7:  b2706fed !  7:  ca066beb fs: Invert logic to reduce indentation
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Invert logic to reduce indentation
    +    fs: nxt_fs_mkdir_dirname(): Invert logic to reduce indentation

         This refactor isn't very appealing alone, but it prepares the code for
         the following commits.
 8:  5d3a1fd4 !  8:  d8dec4a7 fs: Correctly handle "/" in nxt_fs_mkdir_parent()
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: Correctly handle "/" in nxt_fs_mkdir_parent()
    +    fs: nxt_fs_mkdir_dirname(): Correctly handle "/"

         The previous code attempted to mkdir(""), that is an empty string.
         Since "/" necessarily exists, just goto out_free.
 9:  5ab148a8 =  9:  a047d110 fs: Make the full directory path for the pid file and the control socket
10:  c8e54495 = 10:  11aee2c4 auto: Don't install $runstatedir
11:  66c94bc2 = 11:  4bd7f5e7 Use octal instead of mode macros

ac000 commented 2 months ago

Please see my previous comment about prefixes...

alejandro-colomar commented 2 months ago

v2f changes:

re-format subject lines regarding function names

$ git range-diff master gh/fs fs 
 1:  f497b51f =  1:  f497b51f fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()
 2:  44e2b3b8 =  2:  44e2b3b8 fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()
 3:  02fedfb9 !  3:  a783f961 fs: nxt_fs_mkdir_p(): Use branchless code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_p(): Use branchless code
    +    fs: Use branchless code in nxt_fs_mkdir_p()

         That branch was to avoid an infinite loop on the slash.  However, we can
         achieve the same by using a +1 to make sure we advance at least 1 byte
 4:  12e79b48 !  4:  51e36812 fs: nxt_fs_mkdir_p(): Use a temporary variable
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_p(): Use a temporary variable
    +    fs: Use a temporary variable in nxt_fs_mkdir_p()

         This avoids breaking a long line.

 5:  8cba8b34 !  5:  4ecf4c9d fs: nxt_fs_mkdir_p(): Accept relative paths
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_p(): Accept relative paths
    +    fs: Accept relative paths in nxt_fs_mkdir_p()

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
 6:  49867886 !  6:  4c7767cd fs: nxt_fs_mkdir_p(): Accept path names of length 1
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_p(): Accept path names of length 1
    +    fs: Accept path names of length 1 in nxt_fs_mkdir_p()

         That is, accept "/", or relative path names of a single byte.

 7:  ca066beb !  7:  30ea9a6d fs: nxt_fs_mkdir_dirname(): Invert logic to reduce indentation
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_dirname(): Invert logic to reduce indentation
    +    fs: Invert logic to reduce indentation in nxt_fs_mkdir_dirname()

         This refactor isn't very appealing alone, but it prepares the code for
         the following commits.
 8:  d8dec4a7 !  8:  0b8655fe fs: nxt_fs_mkdir_dirname(): Correctly handle "/"
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    fs: nxt_fs_mkdir_dirname(): Correctly handle "/"
    +    fs: Correctly handle "/" in nxt_fs_mkdir_dirname()

         The previous code attempted to mkdir(""), that is an empty string.
         Since "/" necessarily exists, just goto out_free.
 9:  a047d110 =  9:  2983b658 fs: Make the full directory path for the pid file and the control socket
10:  11aee2c4 = 10:  90e1b923 auto: Don't install $runstatedir
11:  4bd7f5e7 = 11:  419eab47 Use octal instead of mode macros

alejandro-colomar commented 2 months ago

Bogus ruby 3.2 test

alejandro-colomar commented 2 months ago

The prefixes are just supposed to be general topics/area's of the code, so I wouldn't put a function name in there...

Ideally, directories and file names and function names should describe topics/area's of the code quite well, so both approaches shouldn't be opposite. :-)

However, this project has a too flat directory structure, which doesn't help.

ac000 commented 2 months ago

My intention for the prefixes/topics is to do it this way

thing: ...

thing/subthing: ...

or

thing: subthing: ...

(Though those are perhaps not too useful in a project of Units size)

thing1, thing2: ...

For when a change effects multiple areas. E.g https://github.com/nginx/unit/pull/1228/commits/347016a818fc649bfa754104bea59bd261f47f4e

ac000 commented 2 months ago

Overall I think these changes look good.

The last one (using octal instead of the symbolic constants) may be controversial and I'd like to have at least @thresheek 's Acked-by: for the general idea / https://github.com/nginx/unit/pull/1235/commits/2983b6585e2d368b6a9e705e0c62e218ecd736bf

alejandro-colomar commented 2 months ago

fs: Accept path names of length 1 in nxt_fs_mkdir_p()

That is, accept "/", or relative path names of a single byte.

Fixes: 57fc9201c ("Socket: Created control socket & pid file directories.")
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

Hmm, the code in question was like that before the patch this reportedly fixes...

Hmmmm, really? That patch is the first one that does anything mkdir(2), no? BTW, I don't mean you broke anything in that patch, but rather that this one "Improves:" yours, but for simplicity I reused that tag. I might as well remove the tag, if you prefer, but I think it's interesting to have a pointer to that commit here.

alejandro-colomar commented 2 months ago

fs: Make the full directory path 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
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, let's just create any directories that we need,
with all their parents, at run time.

Fixes: https://github.com/nginx/unit/commit/57fc9201cb91e3d8901a64e3daaaf31684ee5bf5 ("Socket: Created control socket & pid file directories.")
Link: <https://github.com/nginx/unit/issues/742>
Reported-by: Andy Postnikov <apostnikov@gmail.com>
Tested-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>

I'm not sure that warrants a Fixes:. It worked as intended.

Again, I'm not meaning that you broke anything there. More like "This patch improves what was added in that other commit:". I think it's interesting to have some sort of reference to that other commit. If you prefer to use a "References:" tag for such purpose (that one's probably a bad idea, since it's a RFC 822 header; we'd need something else), I'm fine with it; I usually overload "Fixes:" for that.

ac000 commented 2 months ago

fs: Accept path names of length 1 in nxt_fs_mkdir_p()

That is, accept "/", or relative path names of a single byte.

Fixes: 57fc9201c ("Socket: Created control socket & pid file directories.")
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Hmm, the code in question was like that before the patch this reportedly fixes...
Hmmmm, really? That patch is the first one that does anything mkdir(2), no? BTW, I don't mean you broke anything in

No, nxt_fs_mkdir_all() was introduced back in 2020.

$ git blame src/nxt_fs.c
...
e2b53e16c (Tiago Natel de Moura 2020-05-28 14:57:41 +0100 11) nxt_int_t
e2b53e16c (Tiago Natel de Moura 2020-05-28 14:57:41 +0100 12) nxt_fs_mkdir_all(const u_char *dir, mode_t mode)
e2b53e16c (Tiago Natel de Moura 2020-05-28 14:57:41 +0100 13) {
...
e2b53e16c (Tiago Natel de Moura 2020-05-28 14:57:41 +0100 20)     nxt_assert(dirlen < PATH_MAX && dirlen > 1 && dir[0] == '/');

Here's your previous edit to that file...

commit 4ecf4c9dac29ef6404c0369b4dbf05c49ddf59df
Author: Alejandro Colomar <alx@kernel.org>
Date:   Tue Apr 23 23:25:57 2024 +0200

    fs: Accept relative paths in nxt_fs_mkdir_p()

    Tested-by: Andy Postnikov <apostnikov@gmail.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/src/nxt_fs.c b/src/nxt_fs.c
index a02c51af..3c33837c 100644
--- a/src/nxt_fs.c
+++ b/src/nxt_fs.c
@@ -18,7 +18,7 @@ nxt_fs_mkdir_p(const u_char *dir, mode_t mode)

     dirlen = nxt_strlen(dir);

-    nxt_assert(dirlen < PATH_MAX && dirlen > 1 && dir[0] == '/');
+    nxt_assert(dirlen < PATH_MAX && dirlen > 1);

     dst = path;
     start = (char *) dir;

alejandro-colomar commented 2 months ago

Ahhh sorry! I was seeing _parent in the blame, and my brain was confusing it with _p. :-)

ac000 commented 2 months ago

Again, I'm not meaning that you broke anything there. More like "This patch improves what was added in that other commit:". I think it's interesting to have some sort of reference to that other commit. If you prefer to use a "References:" tag for such purpose (that one's probably a bad idea, since it's a RFC 822 header; we'd need something else), I'm fine with it; I usually overload "Fixes:" for that.

I use the kernels definition otherwise it's casting too wide a net.

If your patch fixes a bug in a specific commit, ...

I think it's interesting to have some sort of reference to that other commit.

If you want to reference that commit then just do it in the commit message like you did in 46cef09f296d9a3d54b98331d25920fc6322bea8

alejandro-colomar commented 2 months ago

v2g changes:

Fix blames, and CC @ac000

$ git range-diff master gh/fs fs 
 1:  f497b51f !  1:  6e8bd9d5 fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()
    @@ Commit message
         to mkdir -p, which is too similar to "parent", so it can lead to
         confusion.

    +    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_controller.c ##
 2:  44e2b3b8 !  2:  7efd57a4 fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()
    @@ Commit message
         of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
         should be more straightforward to readers.

    +    Fixes: e2b53e16c60b ("Added "rootfs" feature.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_cgroup.c ##
 3:  a783f961 =  3:  3e30995c fs: Use branchless code in nxt_fs_mkdir_p()
 4:  51e36812 =  4:  0f726a3c fs: Use a temporary variable in nxt_fs_mkdir_p()
 5:  4ecf4c9d =  5:  1d1ae28c fs: Accept relative paths in nxt_fs_mkdir_p()
 6:  4c7767cd !  6:  3e3ca9de fs: Accept path names of length 1 in nxt_fs_mkdir_p()
    @@ Commit message

         That is, accept "/", or relative path names of a single byte.

    -    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
    +    Fixes: e2b53e16c60b ("Added "rootfs" feature.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 7:  30ea9a6d =  7:  9532ef1a fs: Invert logic to reduce indentation in nxt_fs_mkdir_dirname()
 8:  0b8655fe =  8:  adf21c86 fs: Correctly handle "/" in nxt_fs_mkdir_dirname()
 9:  2983b658 =  9:  3ff45b72 fs: Make the full directory path for the pid file and the control socket
10:  90e1b923 = 10:  3eababb7 auto: Don't install $runstatedir
11:  419eab47 = 11:  eba8db3a Use octal instead of mode macros

ac000 commented 2 months ago

$ git range-diff master gh/fs fs 
 1:  f497b51f !  1:  6e8bd9d5 fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()
    @@ Commit message
         to mkdir -p, which is too similar to "parent", so it can lead to
         confusion.

    +    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")

      ## src/nxt_controller.c ##
 2:  44e2b3b8 !  2:  7efd57a4 fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()
    @@ Commit message
         of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
         should be more straightforward to readers.

    +    Fixes: e2b53e16c60b ("Added "rootfs" feature.")

I'm not sure I would class these as bugs. Would we really add a Fixes: tag every time we renamed something? I would consider these more akin to refactoring.

alejandro-colomar commented 2 months ago

Hi Andrew,

I use the kernels definition otherwise it's casting too wide a net.

If your patch fixes a bug in a specific commit, ...

There's also

A Fixes: tag indicates that the patch fixes an issue in a previous commit.

An issue is a broader term, which can mean something like a readability issue, not necessarily a behavior bug.

I think it's interesting to have some sort of reference to that other commit.

If you want to reference that commit then just do it in the commit message like you did in 46cef09

Actually, in that commit I also used "Fixes:" for the three commits. And for trivial patches that only change a function name or something small, writing a long history is a bit overkill.

$ git log master..HEAD | grep -A5 -e ^commit -e Fixes:  | grep -v ^-- | grep -B2 Fixes:
    auto: Don't install $runstatedir

    Fixes: 5a37171f733f ("Added default values for pathnames.")
    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
--
    fs: Make the full directory path for the pid file and the control socket

    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
--
    fs: Correctly handle "/" in nxt_fs_mkdir_dirname()

    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
--
    fs: Accept path names of length 1 in nxt_fs_mkdir_p()

    Fixes: e2b53e16c60b ("Added "rootfs" feature.")
--
    fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()

    Fixes: e2b53e16c60b ("Added "rootfs" feature.")
--
    fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()

    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")

auto: Don't install $runstatedir and fs: Make the full directory path for the pid file and the control socket fix an issue with Alpine.
fs: Correctly handle "/" in nxt_fs_mkdir_dirname() fixes a bug when the user specifies --pid /unit.pid.
fs: Accept path names of length 1 in nxt_fs_mkdir_p() fixes a bug in the function when trying to mkdir "/" (I'm not sure if it was reproducible, but I'd argue that mkdir_p("/") ought to be a no-op success; and anything that deviates is a bug).
And the renames are of course no bugs, but are readability issues (my confusion reading the blame is proof of it ;).

So, IMHO, it's fair game. A bit of a stretch, maybe. :)

Have a lovely night! Alex

alejandro-colomar commented 2 months ago

I'm not sure I would class these as bugs. Would we really add a Fixes: tag every time we renamed something? I would consider these more akin to refactoring.

In shadow, I do use Fixes even for readability improvements, or other trivial refactors. It helped find other places where the same problem was present, because adding the Fixes tag requires some investigation with the blames, and you usually find that the same commit that added a piece of code, added other pieces of code, and introduced the same issue.

So, while it may seem overkill, it requires a little bit more effort, which results in catching other issues that you didn't know about.

ac000 commented 2 months ago

I just don't want to set a precedent for adding these willy nilly and they potentially loose their value, rather keep them for actual bugs...

alejandro-colomar commented 2 months ago

Okay

alejandro-colomar commented 2 months ago

v2e changes:

Remove "Fixes:" from the first two commits. [@ac000 ]

$ git range-diff master gh/fs fs 
 1:  6e8bd9d5 !  1:  09340996 fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()
    @@ Commit message
         to mkdir -p, which is too similar to "parent", so it can lead to
         confusion.

    -    Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
 2:  7efd57a4 !  2:  e8a68a85 fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()
    @@ Commit message
         of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
         should be more straightforward to readers.

    -    Fixes: e2b53e16c60b ("Added "rootfs" feature.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
 3:  3e30995c =  3:  fd3ed9a7 fs: Use branchless code in nxt_fs_mkdir_p()
 4:  0f726a3c =  4:  5217653d fs: Use a temporary variable in nxt_fs_mkdir_p()
 5:  1d1ae28c =  5:  c416ff88 fs: Accept relative paths in nxt_fs_mkdir_p()
 6:  3e3ca9de =  6:  dd704f4e fs: Accept path names of length 1 in nxt_fs_mkdir_p()
 7:  9532ef1a =  7:  24dc2322 fs: Invert logic to reduce indentation in nxt_fs_mkdir_dirname()
 8:  adf21c86 =  8:  739df6c8 fs: Correctly handle "/" in nxt_fs_mkdir_dirname()
 9:  3ff45b72 =  9:  4462a15a fs: Make the full directory path for the pid file and the control socket
10:  3eababb7 = 10:  7606860b auto: Don't install $runstatedir
11:  eba8db3a = 11:  11d26d33 Use octal instead of mode macros

alejandro-colomar commented 2 months ago

@thresheek , please review.

ac000 commented 2 months ago

You can add my

Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>

Where they're not already...

(Also tested the ./configure --prefix=./build use case)

alejandro-colomar commented 2 months ago

Lovely! Thanks.

alejandro-colomar commented 2 months ago

v2f changes:

Tested & reviewed by @ac000

$ git range-diff master gh/fs fs 
 1:  09340996 !  1:  4af5714d fs: Rename nxt_fs_mkdir_parent() => nxt_fs_mkdir_dirname()
    @@ Commit message
         confusion.

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_controller.c ##
 2:  e8a68a85 !  2:  b9164b1b fs: Rename nxt_fs_mkdir_all() => nxt_fs_mkdir_p()
    @@ Commit message
         should be more straightforward to readers.

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_cgroup.c ##
 3:  fd3ed9a7 !  3:  76547576 fs: Use branchless code in nxt_fs_mkdir_p()
    @@ Commit message
         achieve the same by using a +1 to make sure we advance at least 1 byte
         in each iteration.

    -    Tested-by: Andrew Clayton <a.clayton@nginx.com>
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 4:  5217653d !  4:  30d86ba3 fs: Use a temporary variable in nxt_fs_mkdir_p()
    @@ Commit message
         This avoids breaking a long line.

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 5:  c416ff88 !  5:  5bad130b fs: Accept relative paths in nxt_fs_mkdir_p()
    @@ Commit message
         fs: Accept relative paths in nxt_fs_mkdir_p()

         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 6:  dd704f4e !  6:  7f66687a fs: Accept path names of length 1 in nxt_fs_mkdir_p()
    @@ Commit message

         Fixes: e2b53e16c60b ("Added "rootfs" feature.")
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_fs.c ##
 7:  24dc2322 !  7:  c10f572f fs: Invert logic to reduce indentation in nxt_fs_mkdir_dirname()
    @@ Commit message

         Link: <https://github.com/nginx/unit/issues/742>
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: 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>
 8:  739df6c8 !  8:  73166a92 fs: Correctly handle "/" in nxt_fs_mkdir_dirname()
    @@ Commit message
         Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
         Link: <https://github.com/nginx/unit/issues/742>
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: 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>
 9:  4462a15a !  9:  cdde66f7 fs: Make the full directory path for the pid file and the control socket
    @@ Commit message
         Link: <https://github.com/nginx/unit/issues/742>
         Reported-by: Andy Postnikov <apostnikov@gmail.com>
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: 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>
10:  7606860b ! 10:  9ff0ba82 auto: Don't install $runstatedir
    @@ Commit message
         Closes: <https://github.com/nginx/unit/issues/742>
         Reported-by: Andy Postnikov <apostnikov@gmail.com>
         Tested-by: Andy Postnikov <apostnikov@gmail.com>
    -    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: 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>
11:  11d26d33 ! 11:  79b777c1 Use octal instead of mode macros
    @@ Commit message

         And we had a mix of both styles; there wasn't really a consistent style.

    +    Tested-by: Andrew Clayton <a.clayton@nginx.com>
    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## auto/shmem ##

alejandro-colomar commented 2 months ago

Boogy ruby test.

callahad commented 1 week ago

This looks reasonable enough to me.

@ac000 do we need anything more before approving and merging?

ac000 commented 1 week ago

Overall I'm OK with it.

I think this comment still applies as @thresheek did reject the idea of creating full directory paths when we did this the first time around...

thresheek commented 1 week ago

I guess my only concern is that typos can go unchecked and create a mess of a hierarchy. Other than that, we'd probably need to adjust some packaging for that change, but that can be fixed after this PR is merged.

alejandro-colomar commented 1 week ago

I guess my only concern is that typos can go unchecked and create a mess of a hierarchy. Other than that, we'd probably need to adjust some packaging for that change, but that can be fixed after this PR is merged.

Hi @thresheek, @ac000 , @callahad ,

Hmmmm, just yesterday, something like that happened to me:

I was using scp(1), and I made a typo in the (remote) destination. I was very lucky that scp(1) rejected the copy instead of creating all parents. I remembered you (@thresheek). :-)

But in this case, I'm not sure of how to do it in a different way. If you have any idea, please let me know.

Anyway, since in this case that's usually specified in the packaging, and not by the end users (or if they do so, it's in a Dockerfile, or something else controlled by git(1) (or else)), I expect it to be unlikely to have typos. It's not like one is running unitd with --pid manually specified all the time; unlike with cp(1) or scp(1).

Have a lovely day! Alex

ac000 commented 1 week ago

I guess my only concern is that typos can go unchecked and create a mess of a hierarchy. Other than that, we'd probably need to adjust some packaging for that change, but that can be fixed after this PR is merged.

@thresheek

In that case can we add your

Acked-by: Konstantin Pavlov <thresh@nginx.com>

to fs: Make the full directory path for the pid file and the control socket ?

nginx / unit

mkdir -p $runstatedir at run time, not install time (round 2) #1235