Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 February 2017 at 10:25, Markus Armbruster <armbru@redhat.com> wrote:
>> Reorder check_strtox_error() to make it obvious that we always store
>> through a non-null @endptr.
>>
>> Transform
>>
>>     if (some error) {
>>         error case ...
>>         err = value for error case;
>>     } else {
>>         normal case ...
>>         err = value for normal case;
>>     }
>>     return err;
>>
>> to
>>
>>     if (some error) {
>>         error case ...
>>         return value for error case;
>>     }
>>     normal case ...
>>     return value for normal case;
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/cutils.c | 89 ++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 45 insertions(+), 44 deletions(-)
>>
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 7d165bc..7442d46 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -265,15 +265,20 @@ int64_t qemu_strtosz(const char *nptr, char **end)
>>  static int check_strtox_error(const char *nptr, char *ep,
>>                                const char **endptr, int eno)
>>  {
>> -    if (eno == 0 && ep == nptr) {
>> -        eno = EINVAL;
>> -    }
>> -    if (!endptr && *ep) {
>> -        return -EINVAL;
>> -    }
>>      if (endptr) {
>>          *endptr = ep;
>>      }
>> +
>> +    /* Turn "no conversion" into an error */
>> +    if (eno == 0 && ep == nptr) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Fail when we're expected to consume the string, but didn't */
>> +    if (!endptr && *ep) {
>> +        return -EINVAL;
>> +    }
>> +
>>      return -eno;
>>  }
>
> Doesn't this change the semantics? Previously, for the
> case of (eno == 0 && ep == nptr) we would both set
> *endptr to ep and return -EINVAL. Now we only return -EINVAL
> and leave *endptr alone.

Behavior before the patch:

    if (eno == 0 && ep == nptr) {
        eno = EINVAL;           // set return value to EINVAL, but continue
    }
    if (!endptr && *ep) {
        return -EINVAL;         // return -EINVAL without setting *endptr
                                // correct because endptr is null
    }
    if (endptr) {
        *endptr = ep;           // set *endptr unless endptr is null
    }
    return -eno;                // *endptr got set unless endptr is null

As you say, we set *endptr unless it's null.

After the patch:

    if (endptr) {
        *endptr = ep;           // set *endptr unless endptr is null
    }

    // no matter what happens below, *endptr got set unless endptr is null

    /* Turn "no conversion" into an error */
    if (eno == 0 && ep == nptr) {
        return -EINVAL;
    }

    /* Fail when we're expected to consume the string, but didn't */
    if (!endptr && *ep) {
        return -EINVAL;
    }

    return -eno;

No change, as far as I can tell.

>                          The comment describing the
> qemu_strtol() API is a bit vague about what exactly we keep
> from the strtol() semantics for "no convertable characters"
> but I would assume that retaining "*endptr is written with
> the original value of nptr" is intentional.

I rewrite the comment PATCH 05.  Relevant part:

 * @nptr may be null, and no conversion is performed then.
 *
 * If no conversion is performed, store @nptr in *@endptr and return
 * -EINVAL.

I guess I should qualify "store @nptr in *@endptr" with "unless @endptr
is null" for completeness.  Anything else to improve?