lib/fs/readlink/: readlinknul(): Use ssize_t to simplify #1205
Conversation
alejandro-colomar
force-pushed
the
rln
branch
from
e08f476 to
0de52cb
Compare
There was a problem hiding this comment.
Overall looks good to me, but I'm not 100% sure about the semantics of mock() (after skimming https://lwn.net/Articles/558106/), or _Generic for that matter.
@ikerexxe could you take a quick look at the last patch in this set?
|
(But obviously +1 from me) |
From what I've experimented with it, my mental model is that // Trick: it will actually return the length, not 0.
will_return(__wrap_vasprintf, 0);so if we execute the above, within and if we execute the following, will_return(__wrap_vasprintf, -1);See the implementation of int
__wrap_vasprintf(char **restrict p, const char *restrict fmt, va_list ap)
{
return mock() == -1 ? -1 : __real_vasprintf(p, fmt, ap);
}which reads that value. |
I wrote a manual page for _Generic(3), if you need it. https://kernel.org/pub/linux/docs/man-pages/book/man-pages-6.11.pdf#_Generic%283%29 And here goes an explanation of why I'm using it in this code:
#define smock() ((intmax_t) mock())However, I wanted to make sure that the original type is #define smock() _Generic(mock(), uintmax_t: (intmax_t) mock())This controlling expression in this _Generic(3) is So, if the return type of One it is true, _Generic(3) evaluates the code under the type selector that matches, so it evaluates as While I could have just tested that |
Thanks! I took that as an Acked-by in v3c (somewhat less than a Reviewed-by, because you weren't 100% comfortable). |
alejandro-colomar
force-pushed
the
rln
branch
3 times, most recently
from
de0cc30 to
ba8fdaa
Compare
@hallyn was the explanation about _Generic and mock() ok? Would you like me to document anything better? (Or change some code?) |
Consistently using a signed type allows us to avoid sign-mismatch diagnostics, while keeping the code simple. It feels weird to accept a ssize_t instead of a size_t, but it's a matter of getting used to it. Another way to achieve this with a single 'len' variable and no casts would be to compare against SIZE_MAX, but that's less readable than -1. Or one could write a SIZE_C() macro a la UINT64_C(), and compare the size_t against SIZE_C(-1), but that's still suboptimal (regarding readability) compared to consistently using signed size types. Fixes: b9d00b6 (2024-12-09; "lib/fs/readlink/readlinknul.h: readlinknul(): Silence warning") Acked-by: Serge Hallyn <[email protected]> Cc: Martin Uecker <[email protected]> Cc: "Robert C. Seacord" <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
It is usually a sign of deep errors. We really want to avoid them. Acked-by: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Acked-by: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Acked-by: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Add a signed wrapper around mock() which returns a signed integer. This makes it possible to compare the return value with literal -1. Acked-by: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
|
Since there have been no more comments, I'll merge. We can continue clarifying the details of _Generic and mock() later if necessary. |
Consistently using a signed type allows us to avoid sign-mismatch diagnostics, while keeping the code simple. It feels weird to accept a ssize_t instead of a size_t, but it's a matter of getting used to it.
Another way to achieve this with a single 'len' variable and no casts would be to compare against SIZE_MAX, but that's less readable than -1. Or one could write a SIZE_C() macro a la UINT64_C(), and compare the size_t against SIZE_C(-1), but that's still suboptimal (regarding readability) compared to consistently using signed size types.
Fixes: b9d00b6 (2024-12-09; "lib/fs/readlink/readlinknul.h: readlinknul(): Silence warning")
Cc: @uecker
Cc: @rcseacord
I've also promoted -Wsign-compare diagnostics to errors in a second commit, since these are usually signs of deeply bogus code.
Long term, I'm considering switching many APIs from size_t to ssize_t. There's too much food in the plate for now, though. As long as
-Werror=sign-comparedoesn't trigger, we shouldn't need to worry too much. (In some cases, we might be silencing them with casts, but those are in old code, so we're not in a hurry to fix those.) :)Revisions:
v2
v3
v3b
v3c
v3d
v3e
v3f