[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.