[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZrHdDXJ8yMPucb1a@cloudsdale.the-delta.net.eu.org>
Date: Tue, 6 Aug 2024 10:21:33 +0200
From: "Haelwenn (lanodan) Monnier" <contact@...ktivis.me>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v2 3/3] signal: add str2sig(3) from POSIX.1-2024
[2024-08-05 14:26:46-0400] Rich Felker:
>On Mon, Aug 05, 2024 at 08:56:07AM +0200, contact@...ktivis.me wrote:
>> From: "Haelwenn (lanodan) Monnier" <contact@...ktivis.me>
>>
>> ---
>> include/signal.h | 1 +
>> src/signal/str2sig.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+)
>> create mode 100644 src/signal/str2sig.c
>>
>> diff --git a/include/signal.h b/include/signal.h
>> index 217cfa08..fd486cd0 100644
>> --- a/include/signal.h
>> +++ b/include/signal.h
>> @@ -235,6 +235,7 @@ void psignal(int, const char *);
>>
>> #define SIG2STR_MAX sizeof("RTMIN+32")
>> int sig2str(int signum, char *str);
>> +int str2sig(const char *restrict str, int *restrict pnum);
>>
>> #endif
>>
>> diff --git a/src/signal/str2sig.c b/src/signal/str2sig.c
>> new file mode 100644
>> index 00000000..b6a4fc23
>> --- /dev/null
>> +++ b/src/signal/str2sig.c
>> @@ -0,0 +1,45 @@
>> +#include <signal.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <stdlib.h>
>> +
>> +int str2sig(const char *restrict str, int *restrict pnum)
>> +{
>> + if (str[0] == '\0') return -1;
>> +
>> + errno = 0;
>> + long signum = strtol(str, NULL, 10);
>> + if (errno == 0 && signum < _NSIG) return (*pnum = signum, 0);
>> +
>> + if (strnlen(str, sizeof *__sys_signame) <= sizeof *__sys_signame)
>> + for (int i = 0; i < sizeof __sys_signame/sizeof *__sys_signame; i++)
>> + if (strncmp(str, __sys_signame[i], sizeof *__sys_signame) == 0)
>> + return (*pnum = i, 0);
>> +
>> + // signal aliases
>> + if (strcmp(str, "IOT") == 0)
>> + return (*pnum = SIGIOT, 0);
>> + if (strcmp(str, "UNUSED") == 0)
>> + return (*pnum = SIGUNUSED, 0);
>
>If there were a lot of these I'd lean towards some table to avoid the
>above, but there seem to be only two that fit this pattern, so what
>you've done is probably best.
>
>> +#if SIGPOLL != SIGIO
>> + if (strcmp(str, "POLL") == 0)
>> + return (*pnum = SIGPOLL, 0);
>> +#endif
>
>I think it's when they're equal, not when they're not-equal, that you
>need to special-case this. Otherwise "POLL" won't be accepted on archs
>where the values are equal. Of course you'd need to special-case both
>if it weren't in the table, but see my comment on sig2str about
>putting POLL in the table when not-equal.
Oups, right
>
>> + if (strcmp(str, "RTMIN") == 0)
>> + return (*pnum = SIGRTMIN, 0);
>> + if (strcmp(str, "RTMAX") == 0)
>> + return (*pnum = SIGRTMAX, 0);
>
>These don't count in the "only two" I mentioned above because the
>results are not a constant translation but runtime-variable.
>
>This does raise a point that we could have SIGRTMIN and SIGRTMAX
>expand to constants at musl build time, since the only way they could
>change is by changing musl. My leaning is kinda not to do that,
>though, because I've toyed with the idea of picking the
>implementation-internal signals dynamically so that nesting in
>qemu-user with some signals already stolen by the outer implementation
>can work. I'm not sure if that's actually practical (it's more of a
>pain than it sounds like, at least) but it's probably best to leave it
>the way you've done it so as not to lock that possibility out.
>
>> + if (strncmp(str, "RTMIN+", 6) == 0 || strncmp(str, "RTMAX-", 6) == 0)
>> + {
>> + errno = 0;
>> + long sigrt = strtol(str+6, NULL, 10);
>> + if(errno != 0 || sigrt < 1 || sigrt > SIGRTMAX - SIGRTMIN) return -1;
>> +
>> + *pnum = str[5] == '+' ? SIGRTMIN + sigrt : SIGRTMAX - sigrt;
>> + return 0;
>> + }
>> +
>> + return -1;
>> +}
>
>The call to strtol isn't validating that there's not additional junk
>after the digits, or that there's not a second sign character or
>whitespace before the digits.
>
>The canonical way to solve this is checking isdigit on the first char
>and using the pend argument to see where parsing stopped, but in this
>case, since the + or - is mandatory and already checked, you could
>pass str+5 instead of str+6 to have it parse the + or - too. This
>would require changing the arithmetic that follows, It also
>complicates accepting 0 if you want to, but the current code rejects
>0, so maybe that's okay?
>
>It's also not clear if you want to accept over-long inputs like
>RTMIN+000001. If not, checking that first char after + or - is in
>range '1' to '9' would probably be the easiest.
>
>Another possibility to consider is that interfacing with strtol is
>sufficiently complicated that it's not worth bothering, and that
>open-coding a-'0' or 10*(a-'0')+(b-'0') is easier (if you don't
>actually intend to accept over-length representations).
>
>Rich
Given POSIX only specifies [1..SIGRTMAX-SIGRTMIN-1] I'm leaning towards
leaving RTMIN+0 and RTMAX-0 out unless it's basically gratis to support,
like when open-coding it.
And I think it's reasonable to expect only 1 or 2-digits for RTMIN+/RTMAX-,
although a for-loop on '0' could be done to skip zero-padding.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
