| Message ID | 1453183061-50484-1-git-send-email-czylin@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-19 at 00:57 -0500, Chester Lin wrote: > Coverity CID 1343309 > > Make GC_FREE reachable in all cases in libxl_get_scheduler() by > eliminating the error-path return and instead storing the error code > in > the returned variable. > > To make this semantically consistent, change the return type of > libxl_get_scheduler() from libxl_scheduler to int, and make a note of > the interpretation of the return value in libxl.h. N.B. This change > breaks neither the API nor the ABI of libxl. > Not that I feel too strong about this, but I would reword this last sentence a bit. In fact, ABI, AFAIK, we don't care. API, someone could argue that it does actually break it, it's just the case that we don't think it breaks it in any ways that we should care. And maybe we should also add a note about the libxl_scheduler enum being (and needing to continue to do so) consistent with what xc_sched_id returns, like it's been done in another patch of this series? Anyway, that's all up for the tools maintainers to judge... The patch seems to me to do what was asked during v1 review, so: > Suggested-by: Ian Campbell <ian.campbell@citrix.com> > Signed-off-by: Chester Lin <czylin@uwaterloo.ca> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Regards, Dario
Chester Lin writes ("[PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()"):
> To make this semantically consistent, change the return type of
> libxl_get_scheduler() from libxl_scheduler to int, and make a note of
> the interpretation of the return value in libxl.h. N.B. This change
> breaks neither the API nor the ABI of libxl.
This is as we discussed, I think, thanks.
One nit:
> -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> +int libxl_get_scheduler(libxl_ctx *ctx)
> {
> - int r, sched;
> + int r, ret;
I see that CODING_STYLE doesn't discuss this directly, but I'm not
very keen on the use of `ret' for this variable name. There is quite
a lot of code elsewhere where `ret' is used simply for libxl error
codes.
I think the name `sched' is fine and that here
> - return ERROR_FAIL;
> - GC_FREE;
> + ret = ERROR_FAIL;
+ sched = ERROR_FAIL;
would be fine.
(But this is a bikeshed style issue that others may disagree about so
please let's see what they think before you rework the patch...)
Ian.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 7f28af8..96b6333 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5583,19 +5583,18 @@ out: return rc; } -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx) +int libxl_get_scheduler(libxl_ctx *ctx) { - int r, sched; + int r, ret; GC_INIT(ctx); - r = xc_sched_id(ctx->xch, &sched); + r = xc_sched_id(ctx->xch, &ret); if (r != 0) { LOGE(ERROR, "getting current scheduler id"); - return ERROR_FAIL; - GC_FREE; + ret = ERROR_FAIL; } GC_FREE; - return sched; + return ret; } static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid, diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 05606a7..6f53376 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -1702,7 +1702,10 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *nodemap); int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap); -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx); +/* A return value less than 0 should be interpreted as a libxl_error, while a + * return value greater than or equal to 0 should be interpreted as a + * libxl_scheduler. */ +int libxl_get_scheduler(libxl_ctx *ctx); /* Per-scheduler parameters */ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
Coverity CID 1343309 Make GC_FREE reachable in all cases in libxl_get_scheduler() by eliminating the error-path return and instead storing the error code in the returned variable. To make this semantically consistent, change the return type of libxl_get_scheduler() from libxl_scheduler to int, and make a note of the interpretation of the return value in libxl.h. N.B. This change breaks neither the API nor the ABI of libxl. Suggested-by: Ian Campbell <ian.campbell@citrix.com> Signed-off-by: Chester Lin <czylin@uwaterloo.ca> --- Changed return type of libxl_get_scheduler in order to return both negative error constants and positive scheduler values. --- tools/libxl/libxl.c | 11 +++++------ tools/libxl/libxl.h | 5 ++++- 2 files changed, 9 insertions(+), 7 deletions(-)