diff mbox

[v3,2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

Message ID 1453682751-34615-1-git-send-email-czylin@uwaterloo.ca (mailing list archive)
State New, archived
Headers show

Commit Message

Chester Lin Jan. 25, 2016, 12:45 a.m. UTC 
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
does not change the API in a way that affects functionality.

The libxl_scheduler enum is consistent with the sched_id return value
of xc_sched_id and this must continue to be true.

Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

---
Changed return type of libxl_get_scheduler in order to return both negative
error constants and positive scheduler values.
---
 tools/libxl/libxl.c | 5 ++---
 tools/libxl/libxl.h | 5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

Comments

Ian Campbell Jan. 25, 2016, 11:59 a.m. UTC | #1 
On Sun, 2016-01-24 at 19:45 -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
> does not change the API in a way that affects functionality.
> 
> The libxl_scheduler enum is consistent with the sched_id return value
> of xc_sched_id and this must continue to be true.
> 
> Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I can't see any sign of v3 versions of #3, #4 or #5/5 in my INBOX, have
they gone astray?

Also, both #1 and #2 seem to have been sent independently using separate
git send-email invocations. Please send them from one single invocation, as
reply to a cover letter (which may be referenced with --in-reply-to, or
written using --compose) such that they are all properly threaded together
into a message thread.

Ian.
Ian Campbell Jan. 26, 2016, 4:52 p.m. UTC | #2 
On Mon, 2016-01-25 at 11:59 +0000, Ian Campbell wrote:
> On Sun, 2016-01-24 at 19:45 -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
> > does not change the API in a way that affects functionality.
> > 
> > The libxl_scheduler enum is consistent with the sched_id return value
> > of xc_sched_id and this must continue to be true.
> > 
> > Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> > Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied.

> I can't see any sign of v3 versions of #3, #4 or #5/5 in my INBOX, have
> they gone astray?
> 
> Also, both #1 and #2 seem to have been sent independently using separate
> git send-email invocations. Please send them from one single invocation, as
> reply to a cover letter (which may be referenced with --in-reply-to, or
> written using --compose) such that they are all properly threaded together
> into a message thread.

I've retired the previous #3..#5 out of my queue (without applying
anything) in anticipation of a resend iff they are still needed.

Ian.
Chester Lin Feb. 2, 2016, 4:39 a.m. UTC | #3 
Quoting Ian Campbell <ian.campbell@citrix.com>:

> On Mon, 2016-01-25 at 11:59 +0000, Ian Campbell wrote:
>> On Sun, 2016-01-24 at 19:45 -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
>> > does not change the API in a way that affects functionality.
>> >
>> > The libxl_scheduler enum is consistent with the sched_id return value
>> > of xc_sched_id and this must continue to be true.
>> >
>> > Suggested-by: Ian Campbell <ian.campbell@citrix.com>
>> > Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
>> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Applied.
>
>> I can't see any sign of v3 versions of #3, #4 or #5/5 in my INBOX, have
>> they gone astray?
>>
>> Also, both #1 and #2 seem to have been sent independently using separate
>> git send-email invocations. Please send them from one single invocation, as
>> reply to a cover letter (which may be referenced with --in-reply-to, or
>> written using --compose) such that they are all properly threaded together
>> into a message thread.
>
> I've retired the previous #3..#5 out of my queue (without applying
> anything) in anticipation of a resend iff they are still needed.
>
> Ian.
>
>

I'm sorry for any confusion. I didn't send patch #3 because I thought  
there was agreement that it would be better to disable the check in  
the tool than make any code changes.
We also do not intend to follow up with the IDL refactoring as per  
your earlier comments. If you would like us to resend any of the  
patches for further debate or archiving, I will.

On behalf of Josh, Harley, and myself, I'd like to thank the  
maintainers for reviewing our patches.
diff mbox

Patch

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9c3cb3e..5a450ef 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5582,7 +5582,7 @@  out:
     return rc;
 }
 
-libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
+int libxl_get_scheduler(libxl_ctx *ctx)
 {
     int r, sched;
 
@@ -5590,8 +5590,7 @@  libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
     r = xc_sched_id(ctx->xch, &sched);
     if (r != 0) {
         LOGE(ERROR, "getting current scheduler id");
-        return ERROR_FAIL;
-        GC_FREE;
+        sched = ERROR_FAIL;
     }
     GC_FREE;
     return sched;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7114491..09fcef3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1711,7 +1711,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,