Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2

[George Dunlap <george.dunlap@citrix.com>]

On 30/09/16 11:30, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2"):
>> This is the remaining part of the plumbing (the libxl
>> one) necessary to be able to change the value of the
>> ratelimit_us parameter online, for Credit2 (like it is
>> already for Credit1).
> 
> Thanks.  I have some coding style nits, all but one of which are
> utterly trivial.  If it hadn't been for the `rc=0' one I would have
> acked this patch as-is.
> 
>> +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
>> +                                   libxl_sched_credit2_params *scinfo)
>> +{
>> +    struct xen_sysctl_credit2_schedule sparam;
>> +    int r, rc;
>> +    GC_INIT(ctx);
>> +
>> +    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
>> +    if (rc) {
>> +        goto out;
>> +    }
> 
> When you respin this, I think IWBNI you could change this to
>       if (rc)
>           goto out;
> or
>       if (rc) goto out;
> both of which are encouraged by the coding style, and are more common
> inside libxl.
> 
> An earlier hunk in this patch does not adjust from a similar construct
> where the { } are becoming newly unnecessary.  You may want to adjust
> that too, but I won't object if you don't feel like it.
> 
>> +
>> +    sparam.ratelimit_us = scinfo->ratelimit_us;
>> +
>> +    r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
>> +    if ( r < 0 ) {
>            ^     ^
> Coding style: libxl does not use the spaces just inside the ( ).
> 
>> +        LOGE(ERROR, "Setting Credit2 scheduler parameters");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    scinfo->ratelimit_us = sparam.ratelimit_us;
>> +
>> + out:
>> +    GC_FREE;
>> +    return rc;
> 
> This should have an explicit assignment rc=0 before the out label.  As
> it is, it will happen to be 0 anyway, so that's a stylistic fix.

If I'm happy with it as-is, and I fix these up before I check them in,
can I put your Ack on it?  Or would you rather have Dario re-send it to
make sure the changes get implemented correctly?

 -George


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