[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit |
Date: |
Tue, 14 Feb 2017 13:58:32 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Peter Maydell <address@hidden> writes:
> On 14 February 2017 at 10:25, Markus Armbruster <address@hidden> 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 <address@hidden>
>> ---
>> 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 address@hidden and return
* -EINVAL.
I guess I should qualify "store @nptr in address@hidden" with "unless @endptr
is null" for completeness. Anything else to improve?
- Re: [Qemu-devel] [PATCH 04/24] tests/test-cutils: Clean up qemu_strtoul() result checks, (continued)
- [Qemu-devel] [PATCH 06/24] util/cutils: Rename qemu_strtoll(), qemu_strtoull(), Markus Armbruster, 2017/02/14
- [Qemu-devel] [PATCH 10/24] tests/test-cutils: Add missing qemu_strtosz()... endptr checks, Markus Armbruster, 2017/02/14
- [Qemu-devel] [PATCH 09/24] QemuOpts: Fix to reject numbers that overflow uint64_t, Markus Armbruster, 2017/02/14
- [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit, Markus Armbruster, 2017/02/14
- Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit, Eric Blake, 2017/02/14
- [Qemu-devel] [PATCH 13/24] tests/test-cutils: Cover qemu_strtosz() around range limits, Markus Armbruster, 2017/02/14
- [Qemu-devel] [PATCH 24/24] QemuOpts: Fix checking of sizes for overflow and trailing crap, Markus Armbruster, 2017/02/14
- [Qemu-devel] [PATCH 20/24] qemu-img: Wrap cvtnum() around qemu_strtosz(), Markus Armbruster, 2017/02/14