lib/, src/: Simplify code that calls [sg]etgroups(3) #1123
Conversation
alejandro-colomar
force-pushed
the
add_groups
branch
2 times, most recently
from
0506291 to
94125e4
Compare
|
Does it make sense as a replacement for a simple for loop? I find the utility of such macros questionable. But to the macro: It does multiple expansion of k, n, a which should be avoided. I also do not see why you need the inline function. You could create a compound literal locally &(size_t){ (n) } if you need a pointer here. (BTW: the manpage for lfind should use *.nmemb). Also, if you insist on a generic macro version, why not also make the return type correct? |
I don't like empty loops too much, and I think this improves the readability slightly. But yeah, I also had doubts. We most likely have other places where we do linear search, so I'll probably reuse it elsewhere in the future.
I think it's more questionable if to call lfind(3) in the first place. Once we agree to call lfind(3), this wrapper is probably better, even if just for lfind_(), which already hides the
Hmm, yep, since getgroups(3) returns an int, it doesn't make much sense to use size_t, which forces us to use casts often. Thanks!
Hmmm.
I hadn't thought of it. :) On the other hand, having slimmer macros, and moving as much as possible to a function, is a good thing to do.
I don't understand this. Would you mind suggesting a patch? (Or at least some clarification.)
Yep, thanks! How about this?: $ grepc LFIND .
./lib/search/lfind/lfind.h:#define LFIND(k, a, n, cmp) \
({ \
__auto_type k_ = k; \
__auto_type a_ = a; \
size_t n_ = n; \
size_t *np_; \
\
static_assert(is_same_typeof(k_, a_), ""); \
\
/* lfind(3) $3 wants a 'size_t *n' for historic reasons. */ \
np_ = &(size_t){n_}; \
\
(typeof(k_)) lfind(k_, a_, np_, sizeof(*k_), cmp); \
})$ grepc is_same_typeof .
./lib/must_be.h:#define is_same_typeof(a, b) \
( \
is_same_type(typeof(a), typeof(b)) \
)
$ grepc is_same_type .
./lib/must_be.h:#define is_same_type(a, b) \
( \
__builtin_types_compatible_p(a, b) \
) |
There can be evaluation in typeof / sizeof when used with variably modified types. But I was thinking mainly about macro expansion itself. Maybe this is not a problem here, but if you have several macros nested this can explode quickly.
My local version has [.size * .nmemb] but this is not correct if .nmemb is a pointer, then it should be [.size * (* .nmemb)]
Seems ok to me. Do we need () around the arguments? (not sure) |
Does
Ahhh, I see. That shouldn't be a problem here. Nevertheless, for evaluation reasons it makes sense to avoid it, by using
Ahh, you're right. I'll do
AFAIK, they're redundant. See https://software.codidact.com/posts/290205. BTW, I've reverted to using the inline function lfind_(), to make the macro slimmer. I don't like macros growing too much. I prefer if they are limited to what functions can't do. $ grepc LFIND .
./lib/search/lfind/lfind.h:#define LFIND(k, a, n, cmp) \
({ \
__auto_type k_ = k; \
__auto_type a_ = a; \
\
static_assert(is_same_typeof(k_, a_), ""); \
\
(typeof(k_)) lfind_(k_, a_, n, sizeof(*k_), cmp); \
})$ grepc -tfd lfind_ .
./lib/search/lfind/lfind.h:inline void *
lfind_(const void *k, const void *a, size_t n, size_t ksize,
int (*cmp)(const void *k, const void *elt))
{
// lfind(3) wants a pointer to n for historic reasons.
return lfind(k, a, &n, ksize, cmp);
}I'll push in a moment v2 of this PR. |
No, nothing in the _Generic selector is ever evaluated. |
alejandro-colomar
force-pushed
the
add_groups
branch
from
94125e4 to
430827d
Compare
Thanks! I've pushed v2. |
alejandro-colomar
force-pushed
the
add_groups
branch
12 times, most recently
from
d0f9231 to
ecb74d5
Compare
|
The use of lfind(3) showed that we could also refactor some more to use lsearch(3), which is more compelling as a simplification. Also, I've come up with auto-detection of the right comparison function through a _Generic(3) selector. :-) $ grepc LSEARCH .
./lib/search/l/lsearch.h:#define LSEARCH(k, a, n) \
({ \
__auto_type k_ = k; \
__auto_type a_ = a; \
\
static_assert(is_same_typeof(k_, a_), ""); \
\
(typeof(k_)) lsearch(k_, a_, n, sizeof(*k_), CMP(typeof(k_)));\
})$ grepc CMP .
./lib/search/cmp/cmp.h:#define CMP(TYPE) \
( \
_Generic((TYPE) 0, \
int *: cmp_int, \
long *: cmp_long, \
unsigned int *: cmp_uint, \
unsigned long *: cmp_ulong \
) \
)That CMP() could grow as needed. Once there's something like void-that-binds, this might get simpler (no need for CMP()), but for now, it's quite nice. |
alejandro-colomar
force-pushed
the
add_groups
branch
4 times, most recently
from
b6d2af1 to
2775c61
Compare
alejandro-colomar
force-pushed
the
add_groups
branch
from
9b15830 to
b81d4a0
Compare
|
Queued after the release of 4.17.0. |
alejandro-colomar
force-pushed
the
add_groups
branch
2 times, most recently
from
152a65d to
e02ad77
Compare
|
WG14 is in tha house, it seems :) |
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
getgroups(0, NULL) returns the number of groups, so that we can allocate at once. This might fail if there's a race and the number of users grows while we're allocating, but if that happens, failing is probably a good thing to do. There was some comment saying it doesn't work on some systems, but according to gnulib, that's only NeXTstep 3.2, which we don't support. Link: <https://www.gnu.org/software/gnulib/manual/html_node/getgroups.html> Signed-off-by: Alejandro Colomar <[email protected]>
These macros are for use with bsearch(3),lfind(3),qsort(3). Signed-off-by: Alejandro Colomar <[email protected]>
The use of typeof() for the function pointer argument was suggested by Jorenar. This improves readability of these complex types. Co-authored-by: Martin Uecker <[email protected]> Cc: Jorenar <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
This will allow using lsearch(3). Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Call it regardless of having added any groups. If the group list is the same that getgroups(3) gave us, setgroups(3) will be a no-op, and it simplifies the surrounding code, by removing the 'added' variable, and allowing to call lsearch(3) instead of lfind(3). Signed-off-by: Alejandro Colomar <[email protected]>
setgroups(2) already performs a test to check if the number of groups is too large. Don't do that ourselves, and also don't do it for every iteration. Just let setgroups(2) do it once. Instead of our check, let's report errors from setgroups(2). Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…agnostics Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
We can calculate an upper bound of the number of added groups by counting the number of delimiters in the string (plus one for the element after the last delimiter). This avoids reallocating +1 in a loop. Signed-off-by: Alejandro Colomar <[email protected]>
Since 'list' is used for a comma/colon-separated-value list, grouplist is incorrect and inconsistent. grouplist is not a list, but an array. Use the more common convention of just using plural. Also, use 'gids' to distinguish it from other group representations. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Autoconf's NEWS file says *** AC_FUNC_GETGROUPS and AC_TYPE_GETGROUPS no longer run test programs. These macros were testing for OS bugs that we believe are at least twenty years in the past. Most operating systems are now trusted to provide an accurate prototype for getgroups in unistd.h, and to implement it as specified in POSIX. Signed-off-by: Alejandro Colomar <[email protected]>
This encapsulates the logic for calling getgroups(3), which requires two calls plus a malloc(3) call to do it correctly. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar
force-pushed
the
add_groups
branch
from
e02ad77 to
7e5c95e
Compare
Queued after #1093
Revisions:
v2
LFIND(). [@uecker]typeof($1). [@uecker]v2b
v2c
v3
v3b
v4
v4b
v4c
v4d
v4e
v4f
v5
v5b
v5c
*).v6
v7
v7b
v8
v9
v9b
v10
v11
gotos (and morereturns). It also improves readability, since uses of the same variables are closer together.v12
v13
v13b
v13c
v14
v14b
v14c
v14d
[[gnu::access()]].v14e
v14f
v14g
v14h
v14i
v14j
v15
v15b