12:52 p.m.
On Mon, Sep 23, 2019 at 12:05:11PM -0500, Eric Blake wrote:
> + int nbdkit_parse_long (const char *what, const char *str, long
*r);
> + int nbdkit_parse_unsigned_long (const char *what,
> + const char *str, unsigned long *r);
Do we really want to encourage the use of parse_long and
parse_unsigned_long? Those differ between 32- and 64-bit platforms, at
which point, it's often better to explicitly call out the intended size.
So I'll say why I added these. It may not be a good reason, but here
goes :-)
There are two external APIs we use which need long types, libcurl
(curl_easy_setopt) and libssh (ssh_options_set). The existing
parameters had type long (I don't think unsigned long is actually
used) which we passed directly to one of these APIs. To use another
type would involve a slightly awkward check + cast, with the danger
that the code would break on the rarely tested 32 bit platform.
...
> + int nbdkit_parse_int16_t (const char *what,
> + const char *str, int16_t *r);
> + int nbdkit_parse_uint16_t (const char *what,
> + const char *str, uint16_t *r);
I guess we can add [u]int8_t variants later if a need arises? Or should
we do it now?
We certainly could. I couldn't see an immediate reason, except maybe
for the ’mbr-id’ parameter.
> + int nbdkit_parse_int32_t (const char *what,
> + const char *str, int32_t *r);
> + int nbdkit_parse_uint32_t (const char *what,
> + const char *str, uint32_t *r);
> + int nbdkit_parse_int64_t (const char *what,
> + const char *str, int64_t *r);
> + int nbdkit_parse_uint64_t (const char *what,
> + const char *str, uint64_t *r);
> + int nbdkit_parse_ssize_t (const char *what,
> + const char *str, ssize_t *r);
> + int nbdkit_parse_size_t (const char *what,
> + const char *str, size_t *r);
[s]size_t is another one that can differ between 32- and 64-bit
platforms; but I'm fine with having these function, especially since
there are still efforts being considered about an alternative kernel ABI
with 64-bit integers but 32-bit size_t.
Yes I think if we had to drop long or size_t, I'd rather keep the
size_t ones. Those are actually meaningful -- eg. if used for sizing
an internal array.
> +Parse string C<str> into an integer of various types.
These functions
> +parse a decimal, hexadecimal (C<"0x...">) or octal
(C<"0...">) number.
> +
> +On success the functions return C<0> and set C<*r> to the parsed value
> +(unless C<*r == NULL> in which case the result is discarded). On
> +error, C<nbdkit_error> is called and the functions return C<-1>.
> +
> +The C<what> parameter is printed in error messages to provide context.
> +It should usually be a short descriptive string of what you are trying
> +to parse, eg:
> +
> + if (nbdkit_parse_int ("random seed", argv[1], &seed) == -1)
> + return -1;
> +
> +might print an error:
> +
> + random seed: could not parse number: "lalala"
> +
Do we want to make it part of our documented contract that *r is
unchanged if we return -1?
Yes I think we can document this. It's useful to have that behaviour
defined because it makes error recovery simpler. I will add it to my
copy.
> @@ -129,22 +129,20 @@ cache_config (nbdkit_next_config *next,
void *nxdata,
> return 0;
> }
> else if (strcmp (key, "cache-high-threshold") == 0) {
> - if (sscanf (value, "%d", &hi_thresh) != 1) {
> - nbdkit_error ("invalid cache-high-threshold parameter: %s",
value);
> + if (nbdkit_parse_unsigned ("cache-high-threshold",
> + value, &hi_thresh) == -1)
Semantic change; previously "010" was '10' and "08" was
'8', now "010"
is '8' and "08" is a failure (because we used "%d" instead of
"%i").
(Multiple spots in this patch). Not a big deal to me, but we may want
to call it out in the commit message as intentional.
I guess %i (not %d) is the one which parses octal and hex. I think
this change is for the better however since it improves consistency,
so I'll mention it in the commit message.
> +++ b/include/nbdkit-common.h
> @@ -84,6 +84,28 @@ extern void nbdkit_vdebug (const char *msg, va_list args);
> extern char *nbdkit_absolute_path (const char *path);
> extern int64_t nbdkit_parse_size (const char *str);
> extern int nbdkit_parse_bool (const char *str);
> +extern int nbdkit_parse_int (const char *what, const char *str,
> + int *r);
Should we mark 'what' and 'str' as being non-null parameters? But you
definitely document 'r' as valid when NULL.
I think we decided to avoid using __attribute__((nonnull)) on the
external API, because it was ... unsafe? I don't recall now if we
avoided it deliberately or not, but we definitely don't use it except
on internal functions!
Yes it would make sense as long as it's not unsafe.
> +++ b/plugins/curl/curl.c
> @@ -204,7 +204,9 @@ curl_config (const char *key, const char *value)
> }
>
> else if (strcmp (key, "timeout") == 0) {
> - if (sscanf (value, "%ld", &timeout) != 1 || timeout < 0) {
> + if (nbdkit_parse_long ("timeout", value, &timeout) == -1)
> + return -1;
> + if (timeout < 0) {
> nbdkit_error ("'timeout' must be 0 or a positive timeout in
seconds");
> return -1;
Why not change to 'unsigned timeout'? 'long' is rather oversized on
64-bit platforms, and keeping things signed doesn't buy much compared to
just going unsigned. (Yep, that's me trying to get rid of a need for
parse_long).
See above about curl_easy_setopt.
> +++ b/plugins/partitioning/partitioning.c
> @@ -211,10 +211,8 @@ partitioning_config (const char *key, const char *value)
> else if (strcmp (key, "mbr-id") == 0) {
> if (strcasecmp (value, "default") == 0)
> mbr_id = DEFAULT_MBR_ID;
> - else if (sscanf (value, "%i", &mbr_id) != 1) {
> - nbdkit_error ("could not parse mbr-id: %s", value);
> + else if (nbdkit_parse_int ("mbr-id", value, &mbr_id) == -1)
> return -1;
Is 'int' the best type for this?
Probably not, see above.
> +++ b/plugins/ssh/ssh.c
> @@ -59,7 +59,7 @@ static bool verify_remote_host = true;
> static const char *known_hosts = NULL;
> static const char **identity = NULL;
> static size_t nr_identities = 0;
> -static long timeout = 0;
> +static unsigned long timeout = 0;
Again, a 32-bit timeout regardless of 32- or 64-bit platform is probably
saner.
See above about ssh_options_set.
> +++ b/plugins/vddk/vddk.c
> @@ -271,10 +271,8 @@ vddk_config (const char *key, const char *value)
> return -1;
> }
> else if (strcmp (key, "nfchostport") == 0) {
> - if (sscanf (value, "%d", &nfc_host_port) != 1) {
> - nbdkit_error ("cannot parse nfchostport: %s", value);
> + if (nbdkit_parse_int ("nfchostport", value, &nfc_host_port) ==
-1)
> return -1;
> - }
Is 'int' the right type here?
Actually this is a bug twice over. The VDDK struct wants uint32_t for
both of the port fields. Obviously since they are port numbers
something like uint16_t would be more suitable. (0 has the special
meaning of "default port").
I think I'll change this to uint16_t because the implicit cast up to
uint32_t when we assign it to the VDDK struct shouldn't be a problem,
and it's useful to have bounds checking.
> +/* Functions for parsing signed integers. */
> +int
> +nbdkit_parse_int (const char *what, const char *str, int *rp)
> +{
> + long r;
> + char *end;
> +
> + errno = 0;
> + r = strtol (str, &end, 0);
> +#if INT_MAX != LONG_MAX
> + if (r < INT_MIN || r > INT_MAX)
> + errno = ERANGE;
> +#endif
> + PARSE_COMMON_TAIL;
> +}
Looks correct.
I actually compiled this and ran the tests with -m32 and -m64. It
took several rounds to get it right.
> +#endif
> + if (r < SSIZE_MIN || r > SSIZE_MAX)
> + errno = ERANGE;
> +#endif
> + PARSE_COMMON_TAIL;
> +}
> +
> +/* Functions for parsing unsigned integers. */
> +
> +/* strtou* functions have surprising behaviour if the first character
> + * (after whitespace) is '-', so deny this.
> + */
> +#define PARSE_ERROR_IF_NEGATIVE \
> + do { \
> + const char *p = str; \
> + while (isspace (*p)) \
> + p++; \
> + if (*p == '-') { \
> + nbdkit_error ("%s: negative numbers are not allowed", what);
\
> + return -1; \
> + } \
> + } while (0)
> +
Agree. However, why not modify str in-place, so that the subsequent
strtoul() does not have to repeat our prefix scan?
Yes, that's a good idea, I didn't think it through.
> + PARSE_COMMON_TAIL;
> +}
> +
> +int
> +nbdkit_parse_size_t (const char *what, const char *str, size_t *rp)
> +{
> + unsigned long long r;
> + char *end;
> +
> + PARSE_ERROR_IF_NEGATIVE;
> + errno = 0;
> + r = strtoull (str, &end, 0);
> +#if SIZE_MAX != ULONGLONG_MAX
> + if (r > SIZE_MAX)
> + errno = ERANGE;
> +#endif
> + PARSE_COMMON_TAIL;
> +}
> +
> /* Parse a string as a size with possible scaling suffix, or return -1
> * after reporting the error.
> */
Hmm, seeing this comment, do we really need raw [s]size_t parsing, or
can we get away with scaled parsing anywhere that sizes are intended?
But the code looks correct if you want to keep it.
nbdkit_parse_ssize_t is not used. nbdkit_parse_size_t is used for a
single case, ‘xz-max-depth’ which is genuinely an array length, so
size_t is (albeit marginally) useful here.
It's not worth dying on this particular hill, but I think there is
some use for all of these functions.
> diff --git a/server/socket-activation.c
b/server/socket-activation.c
> index b9db67c..50df4ca 100644
> --- a/server/socket-activation.c
> +++ b/server/socket-activation.c
> @@ -59,11 +59,8 @@ get_socket_activation (void)
> s = getenv ("LISTEN_PID");
> if (s == NULL)
> return 0;
> - if (sscanf (s, "%u", &pid) != 1) {
> - fprintf (stderr, "%s: malformed %s environment variable
(ignored)\n",
> - program_name, "LISTEN_PID");
> + if (nbdkit_parse_unsigned ("LISTEN_PID", s, &pid) == -1)
> return 0;
> - }
Possible semantic change: on parse failure, are we still printing
something to stderr, or is nbdkit_error() not effective this early?
Yes nbdkit_error works right from the start. We have already used it
for parsing server argv[].
> +++ b/server/test-public.c
> +static bool
> +test_nbdkit_parse_ints (void)
> +{
> + bool pass = true;
> + char s[64];
> +
> +#define PARSE(TYPE, FORMAT, TEST, RET, EXPECTED) \
> + do { \
> + error_flagged = false; \
> + TYPE i = 0; \
> + int r = nbdkit_parse_##TYPE ("test", TEST, &i);
\
> + if (r != RET || i != EXPECTED) { \
> + fprintf (stderr, \
> + "%s: %d: wrong parse for %s: r=%d i=" FORMAT
"\n", \
> + __FILE__, __LINE__, TEST, r, i); \
> + pass = false; \
> + } \
> + if ((r == -1) != error_flagged) { \
> + fprintf (stderr, \
> + "%s: %d: wrong error message handling for %s\n",
\
> + __FILE__, __LINE__, TEST); \
> + pass = false; \
> + } \
> + } while (0)
> +#define OK 0
> +#define BAD -1
Nice.
But I discovered you cannot do:
#define BAD -1, 0
which is a shame :-(
> +
> + /* Test the basic parsing of decimals, hexadecimal, octal and
> + * negative numbers.
> + */
> + PARSE (int, "%d", "0", OK, 0);
> + PARSE (int, "%d", "1", OK, 1);
> + PARSE (int, "%d", "99", OK, 99);
> + PARSE (int, "%d", "0x1", OK, 1);
> + PARSE (int, "%d", "0xf", OK, 15);
> + PARSE (int, "%d", "0x10", OK, 16);
> + PARSE (int, "%d", "0xff", OK, 255);
> + PARSE (int, "%d", "01", OK, 1);
> + PARSE (int, "%d", "07", OK, 7);
> + PARSE (int, "%d", "010", OK, 8);
> + PARSE (int, "%d", "+0", OK, 0);
> + PARSE (int, "%d", "+99", OK, 99);
> + PARSE (int, "%d", "+0xf", OK, 15);
> + PARSE (int, "%d", "+010", OK, 8);
> + PARSE (int, "%d", "-0", OK, 0);
> + PARSE (int, "%d", "-99", OK, -99);
> + PARSE (int, "%d", "-0xf", OK, -15);
I'd test upper case at least once, such as -0XF here. Testing leading
whitespace would also be nice.
Ah yes, good idea.
> + PARSE (int, "%d", "-010", OK, -8);
> + PARSE (int, "%d", "2147483647", OK, 2147483647); /* INT_MAX
*/
> + PARSE (int, "%d", "-2147483648", OK, -2147483648); /* INT_MIN
*/
> + PARSE (int, "%d", "0x7fffffff", OK, 0x7fffffff);
> + PARSE (int, "%d", "-0x80000000", OK, -0x80000000);
Matches our earlier assumptions of twos-complement targets.
> +
> + /* Test basic error handling. */
> + PARSE (int, "%d", "", BAD, 0);
> + PARSE (int, "%d", "-", BAD, 0);
I might also test "- 0" as BAD.
OK.
> + PARSE (int, "%d", "+", BAD, 0);
> + PARSE (int, "%d", "++", BAD, 0);
> + PARSE (int, "%d", "++0", BAD, 0);
> + PARSE (int, "%d", "--0", BAD, 0);
> + PARSE (int, "%d", "0x", BAD, 0);
I'd also test "08" as BAD.
OK.
> + PARSE (int, "%d", "42x", BAD, 0);
> + PARSE (int, "%d", "42-", BAD, 0);
> + PARSE (int, "%d", "garbage", BAD, 0);
> + PARSE (int, "%d", "2147483648", BAD, 0); /* INT_MAX + 1 */
> + PARSE (int, "%d", "-2147483649", BAD, 0); /* INT_MIN - 1 */
> + PARSE (int, "%d", "999999999999999999999999", BAD, 0);
> + PARSE (int, "%d", "-999999999999999999999999", BAD, 0);
It might also be worth testing "0x1p1" as invalid (and not an accidental
hex-float). Other possible non-integer strings to be sure we don't
accidentally parse include "inf", "nan", "0.0".
OK.
> +
> + /* Test nbdkit_parse_unsigned. */
> + PARSE (unsigned, "%u", "0", OK, 0);
> + PARSE (unsigned, "%u", "1", OK, 1);
> + PARSE (unsigned, "%u", "99", OK, 99);
> + PARSE (unsigned, "%u", "0x1", OK, 1);
> + PARSE (unsigned, "%u", "0xf", OK, 15);
> + PARSE (unsigned, "%u", "0x10", OK, 16);
> + PARSE (unsigned, "%u", "0xff", OK, 255);
> + PARSE (unsigned, "%u", "01", OK, 1);
> + PARSE (unsigned, "%u", "07", OK, 7);
> + PARSE (unsigned, "%u", "010", OK, 8);
> + PARSE (unsigned, "%u", "+0", OK, 0);
> + PARSE (unsigned, "%u", "+99", OK, 99);
> + PARSE (unsigned, "%u", "+0xf", OK, 15);
> + PARSE (unsigned, "%u", "+010", OK, 8);
> + PARSE (unsigned, "%u", "-0", BAD, 0);
> + PARSE (unsigned, "%u", "-99", BAD, 0);
> + PARSE (unsigned, "%u", "-0xf", BAD, 0);
> + PARSE (unsigned, "%u", "-010", BAD, 0);
I'd also test at least " 0" and " -0" (or similar), to prove we
handled
leading spaces correctly.
OK.
> + PARSE (unsigned, "%u", "2147483647", OK,
2147483647); /* INT_MAX */
> + PARSE (unsigned, "%u", "-2147483648", BAD, 0); /* INT_MIN */
> + PARSE (unsigned, "%u", "0x7fffffff", OK, 0x7fffffff);
> + PARSE (unsigned, "%u", "-0x80000000", BAD, 0);
> +
> + /* Test nbdkit_parse_long. */
> + PARSE (long, "%ld", "0", OK, 0);
> + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MAX) != -1);
> + PARSE (long, "%ld", s, OK, LONG_MAX);
> + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MIN) != -1);
> + PARSE (long, "%ld", s, OK, LONG_MIN);
> + PARSE (long, "%ld", "999999999999999999999999", BAD, 0);
> + PARSE (long, "%ld", "-999999999999999999999999", BAD, 0);
snprintf() side effects inside assert(). Tsk tsk - the test will now
fail under -DNDEBUG. And that's part of why I don't think we want to
expose parse_long.
We should probably loudly break or skip the tests if they
are compiled with -DNDEBUG. We're using assert for testing
in a couple of places.
> +++ b/server/usergroup.c
> @@ -97,7 +97,7 @@ parseuser (const char *id)
>
> saved_errno = errno;
>
> - if (sscanf (id, "%d", &val) == 1)
> + if (nbdkit_parse_int ("parseuser", id, &val) == 0)
Is 'int' still the right type for this?
Hopefully it will warn us if uid_t stops being int. (Also
we're assuming pid_t == int).
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW