| Message ID | 20170816125219.5255-10-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Juergen Gross [mailto:jgross@suse.com] > Sent: 16 August 2017 13:52 > To: xen-devel@lists.xenproject.org > Cc: Juergen Gross <jgross@suse.com>; Paul Durrant > <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew > Cooper <Andrew.Cooper3@citrix.com> > 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@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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? Paul > > - n[i++] = simple_strtoul(t, &e, 0); > - if ( *e != '\0' ) > - goto fail; > - } > if ( i != 3 ) > goto fail; > > @@ -1118,10 +1125,11 @@ static void __init parse_viridian_version(char > *arg) > > printk("viridian-version = %#x,%#x,%#x\n", > viridian_major, viridian_minor, viridian_build); > - return; > + return 0; > > fail: > printk(XENLOG_WARNING "Invalid viridian-version, using default\n"); > + return -EINVAL; > } > custom_param("viridian-version", parse_viridian_version); > > -- > 2.12.3
On 21/08/17 10:33, Paul Durrant wrote: >> -----Original Message----- >> From: Juergen Gross [mailto:jgross@suse.com] >> Sent: 16 August 2017 13:52 >> To: xen-devel@lists.xenproject.org >> Cc: Juergen Gross <jgross@suse.com>; Paul Durrant >> <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew >> Cooper <Andrew.Cooper3@citrix.com> >> 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@citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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
> -----Original Message----- > From: Juergen Gross [mailto:jgross@suse.com] > Sent: 21 August 2017 12:02 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com> > Subject: Re: [PATCH v3 09/52] xen/arch/x86/hvm/viridian.c: let custom > parameter parsing routines return errno > > On 21/08/17 10:33, Paul Durrant wrote: > >> -----Original Message----- > >> From: Juergen Gross [mailto:jgross@suse.com] > >> Sent: 16 August 2017 13:52 > >> To: xen-devel@lists.xenproject.org > >> Cc: Juergen Gross <jgross@suse.com>; Paul Durrant > >> <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew > >> Cooper <Andrew.Cooper3@citrix.com> > >> 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@citrix.com> > >> Cc: Jan Beulich <jbeulich@suse.com> > >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> 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. Ok, I don't feel that strongly. If you believe this is the neatest way... Acked-by: Paul Durrant <paul.durrant@citrix.com> > > > Juergen
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 ); - n[i++] = simple_strtoul(t, &e, 0); - if ( *e != '\0' ) - goto fail; - } if ( i != 3 ) goto fail; @@ -1118,10 +1125,11 @@ static void __init parse_viridian_version(char *arg) printk("viridian-version = %#x,%#x,%#x\n", viridian_major, viridian_minor, viridian_build); - return; + return 0; fail: printk(XENLOG_WARNING "Invalid viridian-version, using default\n"); + return -EINVAL; } custom_param("viridian-version", parse_viridian_version);
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@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- 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(-)