Re: [Qemu-devel] [PATCH 1/4] cutils: Add qemu_strtol() wrapper

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/07/2015 03:09, Carlos L. Torres wrote:
> From: "Carlos L. Torres" <carlos.torres@rackspace.com>
> 
> Add wrapper for strtol() function. Include unit tests.
> 
> Signed-off-by: Carlos L. Torres <carlos.torres@rackspace.com>

Very thorough work, just a couple comments below.

> ---
>  include/qemu-common.h |   2 +
>  tests/test-cutils.c   | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/cutils.c         |  58 ++++++++++
>  3 files changed, 352 insertions(+)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 237d654..a0ce7d8 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -161,6 +161,8 @@ int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
>  int qemu_parse_fd(const char *param);
> +int qemu_strtol(const char *nptr, const char **endptr, int base,
> +                long *result);
>  
>  int parse_uint(const char *s, unsigned long long *value, char **endptr,
>                 int base);
> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> index 2a4556d..0e8fdbf 100644
> --- a/tests/test-cutils.c
> +++ b/tests/test-cutils.c
> @@ -226,6 +226,269 @@ static void test_parse_uint_full_correct(void)
>      g_assert_cmpint(i, ==, 123);
>  }
>  
> +static void test_qemu_strtol_correct(void)
> +{
> +    const char *str = "12345 foo";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 12345);
> +    g_assert(endptr == str + 5);
> +}
> +
> +static void test_qemu_strtol_null(void)
> +{
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(NULL, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, -EINVAL);
> +    g_assert(endptr == NULL);
> +}
> +
> +static void test_qemu_strtol_empty(void)
> +{
> +    const char *str = "";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0);

Please assert that endptr == str.

> +}
> +
> +static void test_qemu_strtol_whitespace(void)
> +{
> +    const char *str = "  \t  ";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0);
> +    g_assert(endptr == str);
> +}
> +
> +static void test_qemu_strtol_invalid(void)
> +{
> +    const char *str = "   xxxx  \t abc";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0);
> +    g_assert(endptr == str);
> +}
> +
> +static void test_qemu_strtol_trailing(void)
> +{
> +    const char *str = "123xxx";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 123);
> +    g_assert(endptr == str + 3);
> +}
> +
> +static void test_qemu_strtol_octal(void)
> +{
> +    const char *str = "0123";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 8, &res);

Add a testcase for str = "0123" and base = 0?

> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0123);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_decimal(void)
> +{
> +    const char *str = "0123";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 10, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 123);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_hex(void)
> +{
> +    const char *str = "0123";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 16, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0x123);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_max(void)
> +{
> +    const char *str = g_strdup_printf("%ld", LONG_MAX);
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, LONG_MAX);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_overflow(void)
> +{
> +    const char *str = "99999999999999999999999999999999999999999999";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, -ERANGE);
> +    g_assert_cmpint(res, ==, LONG_MAX);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_underflow(void)
> +{
> +    const char *str = "-99999999999999999999999999999999999999999999";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err  = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, -ERANGE);
> +    g_assert_cmpint(res, ==, LONG_MIN);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_negative(void)
> +{
> +    const char *str = "  \t -321";
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, -321);
> +    g_assert(endptr == str + strlen(str));
> +}
> +
> +static void test_qemu_strtol_full_correct(void)
> +{
> +    const char *str = "123";
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, NULL, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 123);
> +}
> +
> +static void test_qemu_strtol_full_null(void)
> +{
> +    char f = 'X';
> +    const char *endptr = &f;
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(NULL, &endptr, 0, &res);
> +
> +    g_assert_cmpint(err, ==, -EINVAL);
> +    g_assert(endptr == NULL);
> +}
> +
> +static void test_qemu_strtol_full_empty(void)
> +{
> +    const char *str = "";
> +    long res = 999L;
> +    int err;
> +
> +    err =  qemu_strtol(str, NULL, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 0);
> +}
> +
> +static void test_qemu_strtol_full_negative(void)
> +{
> +    const char *str = " \t -321";
> +    long res = 999;
> +    int err;
> +
> +    err = qemu_strtol(str, NULL, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, -321);
> +}
> +
> +static void test_qemu_strtol_full_trailing(void)
> +{
> +    const char *str = "123xxx";
> +    long res;
> +    int err;
> +
> +    err = qemu_strtol(str, NULL, 0, &res);
> +
> +    g_assert_cmpint(err, ==, -EINVAL);
> +}
> +
> +static void test_qemu_strtol_full_max(void)
> +{
> +    const char *str = g_strdup_printf("%ld", LONG_MAX);
> +    long res;
> +    int err;
> +
> +    err = qemu_strtol(str, NULL, 0, &res);
> +
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, LONG_MAX);
> +}
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -247,5 +510,34 @@ int main(int argc, char **argv)
>      g_test_add_func("/cutils/parse_uint_full/correct",
>                      test_parse_uint_full_correct);
>  
> +    /* qemu_strtol() tests */
> +    g_test_add_func("/cutils/qemu_strtol/correct", test_qemu_strtol_correct);
> +    g_test_add_func("/cutils/qemu_strtol/null", test_qemu_strtol_null);
> +    g_test_add_func("/cutils/qemu_strtol/empty", test_qemu_strtol_empty);
> +    g_test_add_func("/cutils/qemu_strtol/whitespace",
> +                    test_qemu_strtol_whitespace);
> +    g_test_add_func("/cutils/qemu_strtol/invalid", test_qemu_strtol_invalid);
> +    g_test_add_func("/cutils/qemu_strtol/trailing", test_qemu_strtol_trailing);
> +    g_test_add_func("/cutils/qemu_strtol/octal", test_qemu_strtol_octal);
> +    g_test_add_func("/cutils/qemu_strtol/decimal", test_qemu_strtol_decimal);
> +    g_test_add_func("/cutils/qemu_strtol/hex", test_qemu_strtol_hex);
> +    g_test_add_func("/cutils/qemu_strtol/max", test_qemu_strtol_max);
> +    g_test_add_func("/cutils/qemu_strtol/overflow", test_qemu_strtol_overflow);
> +    g_test_add_func("/cutils/qemu_strtol/underflow",
> +                    test_qemu_strtol_underflow);
> +    g_test_add_func("/cutils/qemu_strtol/negative", test_qemu_strtol_negative);
> +    g_test_add_func("/cutils/qemu_strtol_full/correct",
> +                    test_qemu_strtol_full_correct);
> +    g_test_add_func("/cutils/qemu_strtol_full/null",
> +                    test_qemu_strtol_full_null);
> +    g_test_add_func("/cutils/qemu_strtol_full/empty",
> +                    test_qemu_strtol_full_empty);
> +    g_test_add_func("/cutils/qemu_strtol_full/negative",
> +                    test_qemu_strtol_full_negative);
> +    g_test_add_func("/cutils/qemu_strtol_full/trailing",
> +                    test_qemu_strtol_full_trailing);
> +    g_test_add_func("/cutils/qemu_strtol_full/max",
> +                    test_qemu_strtol_full_max);
> +
>      return g_test_run();
>  }
> diff --git a/util/cutils.c b/util/cutils.c
> index 5d1c9eb..bfaccd3 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -359,6 +359,64 @@ int64_t strtosz(const char *nptr, char **end)
>  }
>  
>  /**
> + * Helper function for qemu_strto*l() functions.
> + */
> +static int check_strtox_error(const char **next, char *endptr,
> +                              int err)
> +{
> +    if (!next && *endptr) {
> +        return -EINVAL;
> +    }
> +    if (next) {
> +        *next = endptr;
> +    }
> +    return -err;
> +}
> +
> +/**
> + * QEMU wrappers for strtol(), strtoll(), strtoul(), strotull() C functions.
> + *
> + * Convert ASCII string @nptr to a long integer value
> + * from the given @base. Parameters @nptr, @endptr, @base
> + * follows same semantics as strtol() C function.
> + *
> + * Different from strtol() function, if @endptr is not NULL, this

"Unlike the strtol() function," ...

> + * function will return -EINVAL whenever it cannot fully convert
> + * the string in @nptr with given @base to a long. This function returns
> + * the result of the conversion only through the @result parameter.
> + *
> + * If NULL is passed in @endptr, then the whole string in @ntpr
> + * is a number otherwise it returns -EINVAL.
> + *
> + * RETURN VALUE
> + * Different from strtol() function, this wrapper returns either

Same here.

> + * -EINVAL or the errno set by strtol() function (e.g -ERANGE).
> + * If the conversion overflows, -ERANGE is returned, and @result
> + * is set to the max value of the desired type
> + * (e.g. LONG_MAX, LLONG_MAX, ULONG_MAX, ULLONG_MAX). If the case
> + * of underflow, -ERANGE is returned, and @result is set to the min
> + * value of the desired type. For strtol(), strtoll(), @result is set to
> + * LONG_MIN, LLONG_MIN, respectively, and for strtoul(), strtoull() it
> + * is set to 0.
> + */
> +int qemu_strtol(const char *nptr, const char **endptr, int base,
> +                long *result)
> +{
> +    char *p;
> +    int err = 0;
> +    if (!nptr) {
> +        if (endptr) {
> +            *endptr = nptr;
> +        }
> +        err = -EINVAL;
> +    } else {
> +        errno = 0;
> +        *result = strtol(nptr, &p, base);
> +        err = check_strtox_error(endptr, p, errno);
> +    }
> +    return err;
> +}
> +/**
>   * parse_uint:
>   *
>   * @s: String to parse
>