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)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
