Re: [PATCH v5 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2344 bytes --]

On Fri, 2016-02-05 at 14:51 +0000, Wei Liu wrote:
> On Thu, Feb 04, 2016 at 04:50:44PM -0600, Chong Li wrote:
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2b6371d..b843fa5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > 
> > -    SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
> > +    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
> >      case 'd':
> >          dom = optarg;
> >          break;
> >      case 'p':
> > -        period = strtol(optarg, NULL, 10);
> > +        if (p_index > b_index || p_index > v_index) {
> > +            /* budget or vcpuID is missed */
> > +            fprintf(stderr, "Must specify period, budget and
> > vcpuID\n");
> > +            rc = 1;
> > +            goto out;
> > +        }
> > +        if (p_index >= p_size) {
> > +            p_size *= 2;
> > +            periods = xrealloc(periods, p_size);
> > +            if (!periods) {
> 
> xreaalloc can't fail.
> 
> > +                fprintf(stderr, "Failed to realloc periods\n");
> > +                rc = 1;
> > +                goto out;
> > +            }
> > +        }
> > +        periods[p_index++] = strtol(optarg, NULL, 10);
> 
> You mix option parsing and validation in same place. I think you need
> to
> clearly separate them. It's very hard to review a function like this
> one.
> 
Exactly.

So, considering this, and the fact that the libxl API is changing (the
_set_all() function is being added), I don't think there is much point
in reviewing this version of this patch.

So, good work following up on this! I know, it takes more time than one
would think... but interfaces are very important, so it's normal it
takes a bit to get them right (and convince people that that's the
case! :-P).

Looking forward to v6.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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