On 20.04.2023 08:25, Luca Fancellu wrote:
>> On 17 Apr 2023, at 10:41, Jan Beulich <jbeul...@suse.com> wrote:
>> On 12.04.2023 11:49, Luca Fancellu wrote:
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -314,6 +314,31 @@ int parse_boolean(const char *name, const char *s,
>>> const char *e)
>>> return -1;
>>> }
>>>
>>> +int __init parse_signed_integer(const char *name, const char *s, const
>>> char *e,
>>> + long long *val)
>>> +{
>>> + size_t slen, nlen;
>>> + const char *str;
>>> + long long pval;
>>
>> What use is this extra variable?
>
> I’m using pval to avoid using *val in the case we find that the parsed number
> is not good,
> I think it’s better to don’t change the *val if any error come out, what do
> you think?
Caller ought to check the return value before even considering to look
at the value. Then again I can see how, if the address of a global
variable was passed in, that global may be unduly affected. So I guess
what you have is actually okay.
>>> + slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
>>> + nlen = strlen(name);
>>> +
>>> + /* Does s start with name or contains only the name? */
>>> + if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
>>> + return -1;
>>
>> The comment imo wants wording consistently positive or consistently
>> negative. IOW either you say what you're looking for, or you say
>> what you're meaning to reject.
>
> Ok I’ll rephrase to:
>
> /* Check that this is the name we are looking for and the syntax is right */
>
> Is that better?
It is, thanks. Alternatively how about "... and a value was provided"?
Jan
