Peter Maydell <peter.mayd...@linaro.org> writes:
> On 14 February 2017 at 10:25, Markus Armbruster <arm...@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 <arm...@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?
