unit
unit copied to clipboard
nginx
mkdir -p $runstatedir at run time, not install time (round 2)
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
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"}"
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?
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
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
Is there any specific need to have /run/unit if it used for socket and pid-file only?
Is there any specific need to have
/run/unitif 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".
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).
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 <[email protected]>
## 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 <[email protected]>
Cc: Liam Crilly <[email protected]>
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
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 <[email protected]>
+ Tested-by: Andy Postnikov <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Cc: Andrew Clayton <[email protected]>
Cc: Liam Crilly <[email protected]>
Cc: Konstantin Pavlov <[email protected]>
- Cc: Andy Postnikov <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
Cc: Andrew Clayton <[email protected]>
Cc: Liam Crilly <[email protected]>
Cc: Konstantin Pavlov <[email protected]>
- Cc: Andy Postnikov <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
+ Tested-by: Andy Postnikov <[email protected]>
Cc: Andrew Clayton <[email protected]>
Cc: Liam Crilly <[email protected]>
Cc: Konstantin Pavlov <[email protected]>
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 <[email protected]>
+ Tested-by: Andy Postnikov <[email protected]>
Cc: Andrew Clayton <[email protected]>
Cc: Liam Crilly <[email protected]>
Cc: Konstantin Pavlov <[email protected]>
11: 09fd8bc8 = 11: 4ab657ad Use octal instead of mode macros
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 /
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.
fs: Make parents recursively for the pid file and the control socketI'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.
Actually,
--pid /unit.pidwould 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!
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 <[email protected]>
## 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
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/
On Thu, Apr 25, 2024 at 06:49:43AM -0700, Andrew Clayton wrote:
@ac000 commented on this pull request.
fs: Use branchless codeCould we make that
fs: Use branchless code in nxt_fs_mkdir_all()
LGTM
-- https://www.alejandro-colomar.es/
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 treevariable 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/
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 valueCould 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/
The prefixes are just supposed to be general topics/area's of the code, so I wouldn't put a function name in there...
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...
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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## Commit message ##
- fs: Accept relative paths in nxt_fs_mkdir_all()
+ fs: nxt_fs_mkdir_p(): Accept relative paths
Tested-by: Andy Postnikov <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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
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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## Commit message ##
- fs: nxt_fs_mkdir_p(): Accept relative paths
+ fs: Accept relative paths in nxt_fs_mkdir_p()
Tested-by: Andy Postnikov <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
6: 49867886 ! 6: 4c7767cd fs: nxt_fs_mkdir_p(): Accept path names of length 1
@@ Metadata
Author: Alejandro Colomar <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
## 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
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.
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
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
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 <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>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.
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 <[email protected]> Tested-by: Andy Postnikov <[email protected]> Cc: Andrew Clayton <[email protected]> Cc: Liam Crilly <[email protected]> Cc: Konstantin Pavlov <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>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.