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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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: signature.asc
Description: This is a digitally signed message part

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