Re: [Xen-devel] [PATCH v3 33/52] xen/drivers/passthrough/iommu.c: let custom parameter parsing routines return errno
On 23/08/17 11:37, Jan Beulich wrote: On 23.08.17 at 11:27, wrote: >> On 22/08/17 12:04, Jan Beulich wrote: >> On 16.08.17 at 14:52, 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 33/52] xen/drivers/passthrough/iommu.c: let custom parameter parsing routines return errno
>>> On 23.08.17 at 11:27, wrote: > On 22/08/17 12:04, Jan Beulich wrote: > On 16.08.17 at 14:52, 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. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 33/52] xen/drivers/passthrough/iommu.c: let custom parameter parsing routines return errno
On 22/08/17 12:04, Jan Beulich wrote: On 16.08.17 at 14:52, 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. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 33/52] xen/drivers/passthrough/iommu.c: let custom parameter parsing routines return errno
>>> On 16.08.17 at 14:52, 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? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
