mkdir -p $runstatedir at run time, not install time (round 2) #1235
Conversation
|
Not tested. |
alejandro-colomar
changed the title
|
Thank you! getting to test meantime ag shows that defaults still point to |
The default is actually But yeah, assuming that you specify 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 Or do you mean that even if you specify that some things are hardcoded? |
|
v2 changes:
|
|
Thank you! specified EDIT ref https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/64807 |
|
Is there any specific need to have |
The FHS 3.0 :-) 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 You also specify $ ./configure --help | grep '\<statedir'
--statedir=DIR default: "$localstatedir/lib/unit"
--state=DIR [deprecated] synonym for --statedirUnless you want to keep them to be stable even if we change the defaults (hopefully, shouldn't happen, but might). |
|
v2b changes:
|
I transformed this comment into a Thanks! v2c changes:
|
Only if they attempted something bonkers like |
| @@ -1,5 +1,6 @@ | |||
| /* | |||
| * Copyright (C) NGINX, Inc. | |||
| * Copyright 2024, Alejandro Colomar <alx@kernel.org> | |||
There was a problem hiding this comment.
Heh, I'm not sure the below change constitutes a significant contribution...
There was a problem hiding this comment.
I guess I could move it to the next commit, but that one was already better than the previous ones, which are trivial refactors.
There was a problem hiding this comment.
Since that one already accumulates several changes to that page, I guess it's fair.
There was a problem hiding this comment.
(Although, thanks to all the refactors, the next commit is also just 2 lines in that file, and one is a rename :D)
| @@ -58,7 +59,7 @@ nxt_fs_mkdir_dirname(const u_char *path, mode_t mode) | |||
| ret = NXT_OK; | |||
|
|
|||
| ptr = strrchr(dir, '/'); | |||
| if (nxt_slow_path(ptr == NULL)) { | |||
| if (ptr == dir || nxt_slow_path(ptr == NULL)) { | |||
There was a problem hiding this comment.
I wonder if path == "/" should constitute an error?, as that's clearly not what you'd want.
There was a problem hiding this comment.
One may legitimatelly want to --control=/unit.control.sock, if they don't care about FHS and having an organized system.
We probably agree that they should be punished by such an offense, but misbehaving, or even prohibiting, is probably not a fair punishment.
Actually, |
Ahh, yeah, I just pasted the commit message from the old implementation. I'll revise that. |
Heh, OK, but equally bonkers! |
alejandro-colomar
force-pushed
the
fs
branch
2 times, most recently
from
4ab657a to
66c94bc
Compare
|
v2d changes:
|
There was a problem hiding this comment.
fs: Rename function
"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
languages. Also, in the context of mkdir(), "parents" is used to refer
to mkdir -p, which is too similar to "parent", so it can lead to
confusion.
Perhaps rather than simply 'Rename function' you could specify the function name, i.e
fs: Rename nxt_fs_mkdir_parent()`
Same for other rename commit.
| 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]; |
There was a problem hiding this comment.
Personally I'm not fussed about having Christmas tree variable type ordering and it was 'wrong' before, so meh...
( 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)
|
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()
…
|
|
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.
…
|
|
You can add my Where they're not already... (Also tested the |
|
Lovely! Thanks. |
|
v2f changes:
|
|
Boogy ruby test. |
|
This looks reasonable enough to me. @ac000 do we need anything more before approving and merging? |
|
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... |
|
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! |
In that case can we add your to fs: Make the full directory path for the pid file and the control socket ? |
Sure! |
Cheers! Do you want to the do honours and rebase to master? |
"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 languages. Also, in the context of mkdir(), "parents" is used to refer to mkdir -p, which is too similar to "parent", so it can lead to confusion. 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>
"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 should be more straightforward to readers. 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>
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 in each iteration. 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>
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>
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>
That is, accept "/", or relative path names of a single byte. Fixes: e2b53e1 ("Added "rootfs" feature.") 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>
This refactor isn't very appealing alone, but it prepares the code for the following commits. Link: <nginx#742> Tested-by: Andy Postnikov <apostnikov@gmail.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>
The previous code attempted to mkdir(""), that is an empty string.
Since "/" necessarily exists, just goto out_free.
Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Link: <nginx#742>
Tested-by: Andy Postnikov <apostnikov@gmail.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>
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: 57fc920 ("Socket: Created control socket & pid file directories.") Link: <nginx#742> Reported-by: Andy Postnikov <apostnikov@gmail.com> Tested-by: Andy Postnikov <apostnikov@gmail.com> Tested-by: Andrew Clayton <a.clayton@nginx.com> Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Acked-by: Konstantin Pavlov <thresh@nginx.com> Cc: Liam Crilly <liam@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>
Tested-by: Andy Postnikov <apostnikov@gmail.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>
They are more readable. 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>
Sure! :-) v2g
|
|
You're welcome Andrew! |
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: #1227
Closes: #742