Re: [PATCH v5 07/12] xen: enable Dom0 to use SVE feature

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2023 08:25, Luca Fancellu wrote:
>> On 17 Apr 2023, at 10:41, Jan Beulich <jbeulich@xxxxxxxx> 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