Re: [Xen-devel] [PATCH v3 33/52] xen/drivers/passthrough/iommu.c: let custom parameter parsing routines return errno

[Juergen Gross <jgross@xxxxxxxx>]

On 23/08/17 11:37, Jan Beulich wrote:
>>>> On 23.08.17 at 11:27, <jgross@xxxxxxxx> wrote:
>> On 22/08/17 12:04, Jan Beulich wrote:
>>>>>> On 16.08.17 at 14:52, <jgross@xxxxxxxx> wrote:
>>>> @@ -89,44 +89,50 @@ static void __init parse_iommu_param(char *s)
>>>>              s += 3;
>>>>  
>>>>          ss = strchr(s, ',');
>>>> -        if ( ss )
>>>> -            *ss = '\0';
>>>> -
>>>> -        if ( !parse_bool(s) )
>>>> -            iommu_enable = 0;
>>>> -        else if ( !strcmp(s, "force") || !strcmp(s, "required") )
>>>> +        if ( !ss )
>>>> +            ss = strchr(s, '\0');
>>>> +
>>>> +        b = parse_bool(s);
>>>
>>> I don't think this will work as intended for "iommu=yes,...". Did I
>>> perhaps overlook the same issue in some of the earlier patches?
>>
>> I'm just of the opposite opinion: the former implementation didn't work
>> as intended in that case:
>>
>> Let a hypervisor be built with a pre-defined command line "iommu=no".
>> A user supplied command line "iommu=yes" wouldn't change iommu_enable.
>>
>> My patch changes that: setting iommu_enable to 0 or 1 in case
>> parse_bool() succeeded is the correct thing to do.
>>
>> Maybe I should mention this in the commit message, though.
> 
> That's an orthogonal aspect, which I agree with. My comment
> was about parse_bool() expecting a nul-terminated "yes" (or
> alike), though.

Aah, this is indeed a problem.

I believe the best solution would be to modify parse_bool() to take
an optional string end pointer (using NULL would preserve today's
semantics).


Juergen


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