usermod: Add option to automatically find subordinate IDs#1549
usermod: Add option to automatically find subordinate IDs#1549hallyn merged 1 commit intoshadow-maint:masterfrom
Conversation
|
Need to look at the patch, but definitely +1 to the feature. |
richardweinberger
force-pushed
the
rw/usermod_find_subids
branch
from
942af20 to
cf5ac56
Compare
hallyn
left a comment
hallyn
left a comment
There was a problem hiding this comment.
The switch to id_t does need to be explained, thanks.
|
Could you please show a shell session testing this? It looks good. |
I'm glad that you asked, just noticed a bug while inspecting the shell session. :-/ |
The bug is, |
Tools such as useradd(8) automatically select subordinate UID and GID ranges based on settings in login.defs. But when one wants to add subordinate IDs to an existing user, these ranges have to be specified manually using the -w and -v options of usermod(8). Add a new -S / --add-subids option to usermod(8) which will, just like useradd(8), find a range based on the settings in login.defs. Signed-off-by: Richard Weinberger <richard@nod.at>
richardweinberger
force-pushed
the
rw/usermod_find_subids
branch
from
cf5ac56 to
a0cb3d0
Compare
Here you go: |
Bug fixed and PR updated. |
Thanks! |
It would be interesting to grep(1) the login.defs(5) relevant configs in the shell session. Also, I think it could be good to include this in the commit message. |
This is a freshly installed openSUSE Tumbleweed. |
There was a problem hiding this comment.
LGTM.
From a behavior point of view, it looks good.
While there are a few stylistic things I'd like to change, the most important parts are covered, so it's also good to me. And since it's my fault for not having documented the style before, I'll not complain about those minor details.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
You may want to address @hallyn's comment about id_t in the commit message. I personally don't need it, but you may need it for his approval (I don't know). :)
You may also want to include the shell session in the commit message, since it's relatively small; but also feel free to do it or not (having it in the PR can be sufficient).
Also, I'd like someone else to review the patch before merging. I'm never confident enough with new features.
|
On Tue, Feb 24, 2026 at 02:43:21PM -0800, Alejandro Colomar wrote:
@alejandro-colomar commented on this pull request.
> {
unsigned long min, max;
unsigned long count;
- gid_t start;
+ id_t start;
It's to have a consistent signature for the functions that are now used as a callback. This might be good to specify explicitly.
Yes, this should be in the commit message. (Not in the code, of course, since
it's about a change)
thanks,
-serge
|
|
On Tue, Feb 24, 2026 at 11:16:35PM -0800, richardweinberger wrote:
@richardweinberger commented on this pull request.
> @@ -375,6 +375,32 @@ prepend_range(const char *str, struct id_range_list_entry **head)
*head = entry;
return 1;
}
+
+static int
+find_range(struct id_range_list_entry **head,
+ int (*find_fn)(id_t *range_start, unsigned long *range_count))
+{
+ struct id_range_list_entry *entry;
+ struct id_range range;
+ unsigned long count;
+
+ if (find_fn(&range.first, &count) != 0)
Well, if you insist I'll change it.
That said, please document all these rules somewhere. Ideally with check scripts like the Linux kernel has.
Especially since most of the existing code uses a different style than you have in mind, contributing to this project can be very frustrating.
Just to be clear here. While I don't like some of the style guide :), I'm good
with having a consistent guide. I'm good with encouraging people to look at it
and follow it, and I'm good with, let's say, *one* review round that points out
violations of the style guide, so long as there are also other things that need
to be fixed.
But let's make sure to not nag contributors. After core feedback has been
addressed, stylistic things which are not deemed security relevant should not
be brought up. Not new ones, at least.
What I do not want is for people to be having to rebase and force push 10 times,
each time getting new feedback. Give it all in one round of review, after
that stylistic changes are not relevant. We can always touch it up later.
(Maybe if you're worried that contributors will be less inclined to fix them,
the "if you don't do it, I will do it in a separate commit after your code"
threat will help :) )
|
|
On Wed, Feb 25, 2026 at 05:23:51AM -0800, Alejandro Colomar wrote:
@alejandro-colomar approved this pull request.
LGTM.
>From a behavior point of view, it looks good.
While there are a few stylistic things I'd like to change, the most important parts are covered, so it's also good to me. And since it's my fault for not having documented the style before, I'll not complain about those minor details.
```
Reviewed-by: Alejandro Colomar ***@***.***>
```
You may want to address @hallyn's comment about id_t in the commit message. I personally don't need it, but you may need it for his approval (I don't know). :)
You may also want to include the shell session in the commit message, since it's relatively small, but also feel free to do it or not (having it in the PR can be sufficient).
Also, I'd like someone else to review the patch before merging. I'm never confident enough with new features.
Thanks Alex, I will take a look tonight when I'm on my personal
laptop. I don't expect to see any problems, so expect to merge it.
|
4 participants
Tools such as useradd(8) automatically select subordinate UID and GID ranges based on settings in login.defs.
But when one wants to add subordinate IDs to an existing user, these ranges have to be specified manually using the -w and -v options of usermod(8).
Add a new -S / --add-subids option to usermod(8) which will, just like useradd(8), find a range based on the settings in login.defs.