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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/08/16 10:28, 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 ?
> 
> In general I was surprised at how large this patch, and the
> corresponding xl one, was.  Perhaps after the coding style fixes are
> split off the functional patches will be smaller.
> 
> 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..

FWIW, I find that the massive use of #defines to define entire functions
or large code blocks (particularly blocks which define variables or hide
code flowss) make it very difficult to tell what's going on.  When there
is risk of code getting out of sync, then it may well be worth the cost;
but in a case like this, when there is no inter-dependency, and little
shared functionality, it doesn't make sense to unify things more than
Dario already has (e.g., by making a function to check the parameter value).

 -George


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