Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Mon, 2016-08-22 at 10:28 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 14/24] libxl: allow to set the
> ratelimit value online for Credit2"):
> ...
> > 
> > -    rc = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
> > -    if ( rc < 0 ) {
> > -        LOGE(ERROR, "setting sched credit param");
> > -        GC_FREE;
> > -        return ERROR_FAIL;
> > +    r = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
> > +    if ( r < 0 ) {
> > +        LOGE(ERROR, "Setting Credit scheduler parameters");
> > +        rc = ERROR_FAIL;
> > +        goto out;
> 
> I had to read this three times to figure out what the change was.
> 
> It is good that you are fixing the coding style but can you please
> put
> it in a separate patch ?
> 
Done in v2.

> But I wonder whether there will still be lots of rather formulaic
> code
> that could profitably be generalised somehow.  I'd appreciate your
> views on whether that would be possible, and whether it would be a
> good idea..
> 
I've checked, as promised. TBH, as George said already, I don't see
much more room for factoring or generalizing. Certainly, not in libxl.

In xl (that would be next patch, though), I especially dislike
those main_sched_credit(), main_sched_credit2(), etc., but I think
that, given how the interface looks like, there is again few that we
can do (there's some level of generalization and indirection already,
actually).

So, again, apart from splitting coding style and functional changes, I
don't see other ways for improving this patch. If you have more
concrete ides, please share them. :-)

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
https://lists.xen.org/xen-devel