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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel