Re: [Xen-devel] [PATCH v3 09/52] xen/arch/x86/hvm/viridian.c: let custom parameter parsing routines return errno

[Juergen Gross <jgross@xxxxxxxx>]

On 21/08/17 10:33, Paul Durrant wrote:
>> -----Original Message-----
>> From: Juergen Gross [mailto:jgross@xxxxxxxx]
>> Sent: 16 August 2017 13:52
>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Juergen Gross <jgross@xxxxxxxx>; Paul Durrant
>> <Paul.Durrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew
>> Cooper <Andrew.Cooper3@xxxxxxxxxx>
>> Subject: [PATCH v3 09/52] xen/arch/x86/hvm/viridian.c: let custom
>> parameter parsing routines return errno
>>
>> Modify the custom parameter parsing routines in:
>>
>> xen/arch/x86/hvm/viridian.c
>>
>> to indicate whether the parameter value was parsed successfully.
>>
>> Fix an error in the parsing function: up to now it would overwrite the
>> stack in case more than 3 values are specified.
>>
>> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>> ---
>> V3:
>> - dont modify option value in parsing function
>> - fix error in parsing routine
>> ---
>>  xen/arch/x86/hvm/viridian.c | 28 ++++++++++++++++++----------
>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
>> index aa9b87c0ab..2edf9d0b23 100644
>> --- a/xen/arch/x86/hvm/viridian.c
>> +++ b/xen/arch/x86/hvm/viridian.c
>> @@ -1083,7 +1083,7 @@ static int viridian_load_vcpu_ctxt(struct domain *d,
>> hvm_domain_context_t *h)
>>  HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
>>                            viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
>>
>> -static void __init parse_viridian_version(char *arg)
>> +static int __init parse_viridian_version(const char *arg)
>>  {
>>      const char *t;
>>      unsigned int n[3];
>> @@ -1093,17 +1093,24 @@ static void __init parse_viridian_version(char
>> *arg)
>>      n[1] = viridian_minor;
>>      n[2] = viridian_build;
>>
>> -    while ( (t = strsep(&arg, ",")) != NULL )
>> -    {
>> +    do {
>>          const char *e;
>>
>> -        if ( *t == '\0' )
>> -            continue;
>> +        t = strchr(arg, ',');
>> +        if ( !t )
>> +            t = strchr(arg, '\0');
>> +
>> +        if ( *arg && *arg != ',' && i < 3 )
>> +        {
>> +            n[i] = simple_strtoul(arg, &e, 0);
>> +            if ( e != t )
>> +                goto fail;
>> +        }
>> +
>> +        i++;
>> +        arg = t + 1;
>> +    } while ( *t );
> 
> The loop is much neater when strsep() is used. Could you not just add a check 
> for i < 3 at the beginning?

Of course I could. OTOH I don't think the resulting code would be
neater. I can't remove the check for i being 3 after the loop, so
I'd have to add some lines instead of testing i < 3 in the already
present if statement.

In case you are targeting to continue using strsep(): this would
modify the option string. I want to avoid that as I need it unmodified
for being able to use it in the error message of patch 39.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel