* [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2015-12-28 5:16 ` Coverity tidying Joshua Otto
@ 2015-12-28 5:16 ` Joshua Otto
2016-01-04 16:23 ` Ian Campbell
2015-12-28 5:16 ` [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler() Joshua Otto
` (4 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Joshua Otto @ 2015-12-28 5:16 UTC (permalink / raw)
To: ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, Joshua Otto, hjarmstr
To more closely follow the guidelines in CODING_STYLE, store the result
of the libxc call in the local variable r, and the check the result of
the call in a separate statement.
Additionally, change the error log statement to more accurately reflect
the failure. This is the only functional change introduced by this
patch.
Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
tools/libxl/libxl.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..ca4679b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5585,10 +5585,11 @@ out:
libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
{
- libxl_scheduler sched, ret;
GC_INIT(ctx);
- if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
- LOGE(ERROR, "getting domain info list");
+ libxl_scheduler sched;
+ int r = xc_sched_id(ctx->xch, (int *)&sched);
+ if (r != 0) {
+ LOGE(ERROR, "getting current scheduler id");
return ERROR_FAIL;
GC_FREE;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2015-12-28 5:16 ` [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE Joshua Otto
@ 2016-01-04 16:23 ` Ian Campbell
2016-01-05 8:20 ` Dario Faggioli
0 siblings, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-04 16:23 UTC (permalink / raw)
To: Joshua Otto, xen-devel, Ian Jackson
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> To more closely follow the guidelines in CODING_STYLE, store the result
> of the libxc call in the local variable r, and the check the result of
> the call in a separate statement.
I think a far more important aspect of this change is:
don't store the int return value of xc_sched_id into a variable of type
libxl_scheduler
libxl_scheduler is an enum, and hence "int-like", but... still.
I think this is worth mentioning in the commit message, mainly because I'm
only 99% confident this is just a benign oddity rather than an actual
latent bug.
> Additionally, change the error log statement to more accurately reflect
> the failure. This is the only functional change introduced by this
> patch.
Right.
> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
> ---
> tools/libxl/libxl.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9207621..ca4679b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5585,10 +5585,11 @@ out:
>
> libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> {
> - libxl_scheduler sched, ret;
> GC_INIT(ctx);
> - if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> - LOGE(ERROR, "getting domain info list");
> + libxl_scheduler sched;
> + int r = xc_sched_id(ctx->xch, (int *)&sched);
If you were minded to make a further cleanup (i.e. this is totally
optional) then I'm not convinced this case is actually safe, since
libxl_scheduler could potentially be smaller than sizeof(int), or at least
IIRC the C standard give the compiler that option, although I don't know if
gcc will make use of it or if something else (e.g. OS calling convention on
Linux) would make it a non-issue (or I might be totally wrong...).
Safer (and cleaner looking even if I'm wrong) would be to use a temporary
int for the function call and turn it into an enum implicitly in the
return.
> + if (r != 0) {
> + LOGE(ERROR, "getting current scheduler id");
> return ERROR_FAIL;
> GC_FREE;
> }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2016-01-04 16:23 ` Ian Campbell
@ 2016-01-05 8:20 ` Dario Faggioli
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
0 siblings, 1 reply; 51+ messages in thread
From: Dario Faggioli @ 2016-01-05 8:20 UTC (permalink / raw)
To: Ian Campbell, Joshua Otto, xen-devel, Ian Jackson
Cc: george.dunlap, wei.liu2, hjarmstr, czylin, stefano.stabellini
[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]
On Mon, 2016-01-04 at 16:23 +0000, Ian Campbell wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > @@ -5585,10 +5585,11 @@ out:
> >
> > libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> > {
> > - libxl_scheduler sched, ret;
> > GC_INIT(ctx);
> > - if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> > - LOGE(ERROR, "getting domain info list");
> > + libxl_scheduler sched;
> > + int r = xc_sched_id(ctx->xch, (int *)&sched);
>
> Safer (and cleaner looking even if I'm wrong) would be to use a
> temporary
> int for the function call and turn it into an enum implicitly in the
> return.
>
FWIW, +1 to this
Regadrs,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2016-01-05 8:20 ` Dario Faggioli
@ 2016-01-19 5:57 ` Chester Lin
2016-01-19 9:14 ` Dario Faggioli
2016-01-19 11:28 ` Wei Liu
0 siblings, 2 replies; 51+ messages in thread
From: Chester Lin @ 2016-01-19 5:57 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, dario.faggioli,
ian.jackson, Chester Lin, jtotto, hjarmstr
To more closely follow the guidelines in CODING_STYLE, store the result
of xc_sched_id() in the local variable r, and the check the result of
the call in a separate statement. Change the type of the output
parameter given to xc_sched_id() from libxl_scheduler to int to match
the libxc interface.
Additionally, change the error log statement to more accurately reflect
the failure. This is the only functional change introduced by this
patch.
Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
Now storing the return of xc_sched_id in an int as per
On Mon, 2016-01-04 at 16:23 +0000, Ian Campbell wrote:
>Safer (and cleaner looking even if I'm wrong) would be to use a temporary
>int for the function call and turn it into an enum implicitly in the return
---
tools/libxl/libxl.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..7f28af8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5585,10 +5585,12 @@ out:
libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
{
- libxl_scheduler sched, ret;
+ int r, sched;
+
GC_INIT(ctx);
- if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
- LOGE(ERROR, "getting domain info list");
+ r = xc_sched_id(ctx->xch, &sched);
+ if (r != 0) {
+ LOGE(ERROR, "getting current scheduler id");
return ERROR_FAIL;
GC_FREE;
}
--
1.9.5.msysgit.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
@ 2016-01-19 9:14 ` Dario Faggioli
2016-01-19 11:28 ` Wei Liu
1 sibling, 0 replies; 51+ messages in thread
From: Dario Faggioli @ 2016-01-19 9:14 UTC (permalink / raw)
To: Chester Lin, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, jtotto,
hjarmstr
[-- Attachment #1.1: Type: text/plain, Size: 992 bytes --]
On Tue, 2016-01-19 at 00:57 -0500, Chester Lin wrote:
> To more closely follow the guidelines in CODING_STYLE, store the
> result
> of xc_sched_id() in the local variable r, and the check the result of
> the call in a separate statement. Change the type of the output
> parameter given to xc_sched_id() from libxl_scheduler to int to match
> the libxc interface.
>
> Additionally, change the error log statement to more accurately
> reflect
> the failure. This is the only functional change introduced by this
> patch.
>
> 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
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
2016-01-19 9:14 ` Dario Faggioli
@ 2016-01-19 11:28 ` Wei Liu
2016-01-19 11:35 ` Ian Campbell
1 sibling, 1 reply; 51+ messages in thread
From: Wei Liu @ 2016-01-19 11:28 UTC (permalink / raw)
To: Chester Lin
Cc: wei.liu2, ian.campbell, stefano.stabellini, dario.faggioli,
ian.jackson, xen-devel, jtotto, hjarmstr
Hi Chester
What we normally do for new version of patches is to send out a new
series prefixed with "PATCH v2", instead of replying to old version of
the patches.
Could you collect Dario's Reviewed-by tags and send this series as v2.
Thanks
Wei.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
2016-01-19 11:28 ` Wei Liu
@ 2016-01-19 11:35 ` Ian Campbell
0 siblings, 0 replies; 51+ messages in thread
From: Ian Campbell @ 2016-01-19 11:35 UTC (permalink / raw)
To: Wei Liu, Chester Lin
Cc: stefano.stabellini, dario.faggioli, ian.jackson, xen-devel,
jtotto, hjarmstr
On Tue, 2016-01-19 at 11:28 +0000, Wei Liu wrote:
> Hi Chester
>
> What we normally do for new version of patches is to send out a new
> series prefixed with "PATCH v2", instead of replying to old version of
> the patches.
>
> Could you collect Dario's Reviewed-by tags and send this series as v2.
Please call the resend v3 to avoid any confusion.
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Review.2C_Rinse_.26_Repeat
also has some words on this topic.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2015-12-28 5:16 ` Coverity tidying Joshua Otto
2015-12-28 5:16 ` [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE Joshua Otto
@ 2015-12-28 5:16 ` Joshua Otto
2016-01-04 16:29 ` Ian Campbell
2015-12-28 5:16 ` [PATCH 3/5] ns16550: widen an integer constant for Coverity Joshua Otto
` (3 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Joshua Otto @ 2015-12-28 5:16 UTC (permalink / raw)
To: ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, Joshua Otto, hjarmstr
Coverity CID 1343309
This patch preserves the multiple error paths in order to avoid
meaninglessly assigning the ERROR_FAIL libxl error code to the
return variable, which is of type libxl_scheduler.
Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
tools/libxl/libxl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ca4679b..60a2509 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5590,8 +5590,8 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
int r = xc_sched_id(ctx->xch, (int *)&sched);
if (r != 0) {
LOGE(ERROR, "getting current scheduler id");
- return ERROR_FAIL;
GC_FREE;
+ return ERROR_FAIL;
}
GC_FREE;
return sched;
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2015-12-28 5:16 ` [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler() Joshua Otto
@ 2016-01-04 16:29 ` Ian Campbell
2016-01-05 8:49 ` Dario Faggioli
0 siblings, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-04 16:29 UTC (permalink / raw)
To: Joshua Otto, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> Coverity CID 1343309
>
> This patch preserves the multiple error paths in order to avoid
> meaninglessly assigning the ERROR_FAIL libxl error code to the
> return variable, which is of type libxl_scheduler.
Which makes me think that the existing code is bogus to return an error
code as a libxl_scheduler too, since that is not very different to the
bogus assignment.
The function ought to return LIBXL_SCHEDLER_UNKNOWN. However that might be
considered an API breakage (since at least xl checks for errors with < 0).
A _really_ correct libxl function API would take an output libxl_scheduler
pointer and return an int error code, but that is definitely an API change.
Given that a caller really ought to be handling LIBXL_SCHEDULER_UNKNOWN as
a return value, even if it is also written to expect negative error values
as well, so I reckon we can get away with changing the return to
SCHEDULER_UNKNOWN in the error case. Either the caller already handles
that, or it was already buggy in not doing so.
That would also allow us to move to a single error path, since you'd no
longer worry about clobbering.
Wei, Ian, any thoughts?
> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
> ---
> tools/libxl/libxl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ca4679b..60a2509 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5590,8 +5590,8 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> int r = xc_sched_id(ctx->xch, (int *)&sched);
> if (r != 0) {
> LOGE(ERROR, "getting current scheduler id");
> - return ERROR_FAIL;
> GC_FREE;
> + return ERROR_FAIL;
> }
> GC_FREE;
> return sched;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2016-01-04 16:29 ` Ian Campbell
@ 2016-01-05 8:49 ` Dario Faggioli
2016-01-05 11:16 ` Ian Campbell
0 siblings, 1 reply; 51+ messages in thread
From: Dario Faggioli @ 2016-01-05 8:49 UTC (permalink / raw)
To: Ian Campbell, Joshua Otto, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, ian.jackson, czylin,
hjarmstr
[-- Attachment #1.1: Type: text/plain, Size: 2370 bytes --]
On Mon, 2016-01-04 at 16:29 +0000, Ian Campbell wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> > Coverity CID 1343309
> >
> > This patch preserves the multiple error paths in order to avoid
> > meaninglessly assigning the ERROR_FAIL libxl error code to the
> > return variable, which is of type libxl_scheduler.
>
> Which makes me think that the existing code is bogus to return an
> error
> code as a libxl_scheduler too, since that is not very different to
> the
> bogus assignment.
>
Indeed.
> Given that a caller really ought to be handling
> LIBXL_SCHEDULER_UNKNOWN as
> a return value, even if it is also written to expect negative error
> values
> as well, so I reckon we can get away with changing the return to
> SCHEDULER_UNKNOWN in the error case. Either the caller already
> handles
> that, or it was already buggy in not doing so.
>
Again, FWIW, I think this indeed is the proper way forward.
About callers, xl is, of course, quite easy to change.
I just quickly checked libvirt, and I think things will just continue
to work there too. In fact, libxl_get_scheduler() is used 3 times in
there, of which:
- 2 of them, explicitly check for the result to
be LIBXL_SCHEDULER_CREDIT, and errors if it is not (as Credit1 is
the only supported scheduler in libvirt for now)
- 1 explicitly check for the result to be either _SEDF, _CREDIT,
_CREDIT2 or _ARINC653, and errors out if it's something else[1]
For other users, I agree that they should be handling or start to
handle LIBXL_SCHEDULER_UNKNOWN. *Maybe*, just as a "mitigation"
measure", we can redefine the enum and make "unknown"=-1... or is
something like that to be considered an API change as well?
I know, it looks really an hack, and it would remain wrong, for a
caller, to check for a libxl_scheduler object to be equal to
ERROR_FAIL, but maybe it's worth at least being considered.
Thanks and Regards,
Dario
[1] note to self, send a patch to update that (e.g., adding RTDS and
removing SEDF, when adequate, depending on Xen version)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2016-01-05 8:49 ` Dario Faggioli
@ 2016-01-05 11:16 ` Ian Campbell
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
0 siblings, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-05 11:16 UTC (permalink / raw)
To: Dario Faggioli, Joshua Otto, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, ian.jackson, czylin,
hjarmstr
On Tue, 2016-01-05 at 09:49 +0100, Dario Faggioli wrote:
> For other users, I agree that they should be handling or start to
> handle LIBXL_SCHEDULER_UNKNOWN. *Maybe*, just as a "mitigation"
> measure", we can redefine the enum and make "unknown"=-1... or is
> something like that to be considered an API change as well?
So in theory an enum can be signed per the spec (and is required to be if
any of the tags are assigned a -ve value) and it also seems like it is
signed in the ABIs we support (otherwise "sched < 0" would surely generate
warnings on some compiler).
However I don't think we want to change the value of SCHEDULER_UNKNOWN away
from 0 since we expect in many parts of the libxl API that things can be
checked for validity with ! (e.g. if (!sched) barf()) I haven't looked at
whether anyone does in this specific case.
I spoke with Ian J IRL and he suggested that libxl_get_scheduler should
probably return an explicit int, which can contain either the (negative)
libxl ERROR_* constants or the (positive) values of libxl_scheduler.
As far as we can tell there are no worry ABI or API considerations from
such a change (there are corner cases, such as function pointers no longer
being seen as compatible, but we think those won't be an issue in this
case).
Ian.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2016-01-05 11:16 ` Ian Campbell
@ 2016-01-19 5:57 ` Chester Lin
2016-01-19 9:08 ` Dario Faggioli
2016-01-19 14:15 ` Ian Jackson
0 siblings, 2 replies; 51+ messages in thread
From: Chester Lin @ 2016-01-19 5:57 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, dario.faggioli,
ian.jackson, Chester Lin, jtotto, hjarmstr
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(-)
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,
--
1.9.5.msysgit.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
@ 2016-01-19 9:08 ` Dario Faggioli
2016-01-19 14:15 ` Ian Jackson
1 sibling, 0 replies; 51+ messages in thread
From: Dario Faggioli @ 2016-01-19 9:08 UTC (permalink / raw)
To: Chester Lin, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, jtotto,
hjarmstr
[-- Attachment #1.1: Type: text/plain, Size: 1579 bytes --]
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
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()
2016-01-19 5:57 ` [PATCH v2 " Chester Lin
2016-01-19 9:08 ` Dario Faggioli
@ 2016-01-19 14:15 ` Ian Jackson
1 sibling, 0 replies; 51+ messages in thread
From: Ian Jackson @ 2016-01-19 14:15 UTC (permalink / raw)
To: Chester Lin
Cc: wei.liu2, ian.campbell, stefano.stabellini, dario.faggioli,
xen-devel, jtotto, hjarmstr
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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 3/5] ns16550: widen an integer constant for Coverity.
2015-12-28 5:16 ` Coverity tidying Joshua Otto
2015-12-28 5:16 ` [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE Joshua Otto
2015-12-28 5:16 ` [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler() Joshua Otto
@ 2015-12-28 5:16 ` Joshua Otto
2016-01-04 16:36 ` Ian Campbell
2015-12-28 5:16 ` [PATCH 4/5] credit: remove pointless local variable initialization Joshua Otto
` (2 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Joshua Otto @ 2015-12-28 5:16 UTC (permalink / raw)
To: ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
From: Harley Armstrong <hjarmstr@uwaterloo.ca>
Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
avoid overflow for large values of reg_shift.
Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
---
xen/drivers/char/ns16550.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index bc24015..546bba1 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
* Force length of mmio region to be at least
* 8 bytes times (1 << reg_shift)
*/
- if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+ if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )
continue;
if ( bar_idx >= uart_param[p].max_bars )
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 3/5] ns16550: widen an integer constant for Coverity.
2015-12-28 5:16 ` [PATCH 3/5] ns16550: widen an integer constant for Coverity Joshua Otto
@ 2016-01-04 16:36 ` Ian Campbell
2016-01-06 9:26 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-04 16:36 UTC (permalink / raw)
To: Joshua Otto, xen-devel, Jan Beulich
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
Please check MAINTAINERS (perhaps using ./scripts/get_maintainers.pl) to
determine the maintainers in order to CC them.
(Hrm, I see now in your cover letter you have, I wonder why Jan et al are
missing?)
> From: Harley Armstrong <hjarmstr@uwaterloo.ca>
>
> Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
> avoid overflow for large values of reg_shift.
A reg_shift large enough to actually expose this would be infeasibly large
(since it would imply a UART taking practically the entire virtual address
space of the processor).
So while Coverity is likely correct here, it is probably also a bit
misguided in the context.
I don't especially object to this change as means to quieten coverity, but
perhaps checking for some sane limit to reg_shift would also serve to
quieten coverity?
That would also avoid the need to check for overflow on the multiplication,
assuming a suitable sane limit was chosen.
> Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
> ---
> xen/drivers/char/ns16550.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index bc24015..546bba1 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t
> skip_amt, unsigned int bar_idx)
> * Force length of mmio region to be at least
> * 8 bytes times (1 << reg_shift)
> */
> - if ( size < (0x8 * (1 <<
> uart_param[p].reg_shift)) )
> + if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )
It looks from the surrounding code like
... < (0x8 * ((u64)1 << uart_param[p].reg_shift)) )
would be the preferred way of tackling this.
> continue;
>
> if ( bar_idx >= uart_param[p].max_bars )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/5] ns16550: widen an integer constant for Coverity.
2016-01-04 16:36 ` Ian Campbell
@ 2016-01-06 9:26 ` Jan Beulich
2016-01-19 5:57 ` [PATCH v2 3/5] n16550: add sanity check for reg_shift Chester Lin
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-01-06 9:26 UTC (permalink / raw)
To: Ian Campbell, Joshua Otto
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, xen-devel, hjarmstr
>>> On 04.01.16 at 17:36, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
>> Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
>> avoid overflow for large values of reg_shift.
>
> A reg_shift large enough to actually expose this would be infeasibly large
> (since it would imply a UART taking practically the entire virtual address
> space of the processor).
>
> So while Coverity is likely correct here, it is probably also a bit
> misguided in the context.
>
> I don't especially object to this change as means to quieten coverity, but
> perhaps checking for some sane limit to reg_shift would also serve to
> quieten coverity?
Indeed I'd prefer an alternative change to be found, as 64-bit
arithmetic is quite pointless here.
>> Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
>> ---
>> xen/drivers/char/ns16550.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index bc24015..546bba1 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t
>> skip_amt, unsigned int bar_idx)
>> * Force length of mmio region to be at least
>> * 8 bytes times (1 << reg_shift)
>> */
>> - if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
>> + if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )
>
> It looks from the surrounding code like
> ... < (0x8 * ((u64)1 << uart_param[p].reg_shift)) )
>
> would be the preferred way of tackling this.
If we were to really stay with a change to this line, then the
multiplication should go away altogether, since just a shift
will always suffice.
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 3/5] n16550: add sanity check for reg_shift
2016-01-06 9:26 ` Jan Beulich
@ 2016-01-19 5:57 ` Chester Lin
2016-01-19 13:32 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Chester Lin @ 2016-01-19 5:57 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, Chester Lin, jtotto, JBeulich, hjarmstr
Fix CID 1343302 by adding checking a check on the value of reg_shift.
This patch also rolls the multiplication by 8 into the shift.
No functional changes.
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
xen/drivers/char/ns16550.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index bc24015..55cfc45 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
* Force length of mmio region to be at least
* 8 bytes times (1 << reg_shift)
*/
- if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+ if ( uart_param[p].reg_shift > 27 ||
+ size < (1 << (uart_param[p].reg_shift + 3)) )
continue;
if ( bar_idx >= uart_param[p].max_bars )
--
1.9.5.msysgit.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 3/5] n16550: add sanity check for reg_shift
2016-01-19 5:57 ` [PATCH v2 3/5] n16550: add sanity check for reg_shift Chester Lin
@ 2016-01-19 13:32 ` Jan Beulich
2016-01-25 0:41 ` czylin
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-01-19 13:32 UTC (permalink / raw)
To: Chester Lin
Cc: ian.campbell, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, xen-devel, jtotto, hjarmstr
>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> wrote:
> Fix CID 1343302 by adding checking a check on the value of reg_shift.
> This patch also rolls the multiplication by 8 into the shift.
> No functional changes.
>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
I don't think so.
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> ---
> xen/drivers/char/ns16550.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index bc24015..55cfc45 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
> * Force length of mmio region to be at least
> * 8 bytes times (1 << reg_shift)
> */
> - if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
> + if ( uart_param[p].reg_shift > 27 ||
> + size < (1 << (uart_param[p].reg_shift + 3)) )
> continue;
Instead I think Coverity complaining is mad, and adding a
comparison here just clutters the code. The only thing I could
imagine I might have suggested would be to put an ASSERT()
here.
In any event should is the replacement of the multiplication
by an addition not what I think I had also mentioned before:
The expression, if changed in the first place, should use 8 as
the left operand of the shift.
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 3/5] n16550: add sanity check for reg_shift
2016-01-19 13:32 ` Jan Beulich
@ 2016-01-25 0:41 ` czylin
0 siblings, 0 replies; 51+ messages in thread
From: czylin @ 2016-01-25 0:41 UTC (permalink / raw)
To: Jan Beulich
Cc: ian.campbell, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, xen-devel, jtotto, hjarmstr
Quoting Jan Beulich <JBeulich@suse.com>:
>>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> wrote:
>> Fix CID 1343302 by adding checking a check on the value of reg_shift.
>> This patch also rolls the multiplication by 8 into the shift.
>> No functional changes.
>>
>> Suggested-by: Jan Beulich <JBeulich@suse.com>
>
> I don't think so.
>
>> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
>> ---
>> xen/drivers/char/ns16550.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index bc24015..55cfc45 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t
>> skip_amt, unsigned int bar_idx)
>> * Force length of mmio region to be at least
>> * 8 bytes times (1 << reg_shift)
>> */
>> - if ( size < (0x8 * (1 <<
>> uart_param[p].reg_shift)) )
>> + if ( uart_param[p].reg_shift > 27 ||
>> + size < (1 << (uart_param[p].reg_shift + 3)) )
>> continue;
>
> Instead I think Coverity complaining is mad, and adding a
> comparison here just clutters the code. The only thing I could
> imagine I might have suggested would be to put an ASSERT()
> here.
>
> In any event should is the replacement of the multiplication
> by an addition not what I think I had also mentioned before:
> The expression, if changed in the first place, should use 8 as
> the left operand of the shift.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
Sorry for the confusion regarding the suggested-by tag.
Thank you for reviewing our patches. We agree that it would make
more sense to suppress the error in Coverity. As such, we will
not be sending this patch for further review.
Regards,
Chester Lin
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 4/5] credit: remove pointless local variable initialization.
2015-12-28 5:16 ` Coverity tidying Joshua Otto
` (2 preceding siblings ...)
2015-12-28 5:16 ` [PATCH 3/5] ns16550: widen an integer constant for Coverity Joshua Otto
@ 2015-12-28 5:16 ` Joshua Otto
2015-12-28 5:16 ` [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm Joshua Otto
2015-12-28 9:34 ` Coverity tidying Andrew Cooper
5 siblings, 0 replies; 51+ messages in thread
From: Joshua Otto @ 2015-12-28 5:16 UTC (permalink / raw)
To: ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, Joshua Otto, hjarmstr
Coverity CID 1343301
No functional changes.
Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
xen/common/sched_credit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..02afddf 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1792,7 +1792,7 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
struct csched_private *prv = CSCHED_PRIV(ops);
struct csched_pcpu *spc;
struct csched_vcpu *svc;
- spinlock_t *lock = lock;
+ spinlock_t *lock;
unsigned long flags;
int loop;
#define cpustr keyhandler_scratch
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2015-12-28 5:16 ` Coverity tidying Joshua Otto
` (3 preceding siblings ...)
2015-12-28 5:16 ` [PATCH 4/5] credit: remove pointless local variable initialization Joshua Otto
@ 2015-12-28 5:16 ` Joshua Otto
2016-01-04 16:40 ` Ian Campbell
2015-12-28 9:34 ` Coverity tidying Andrew Cooper
5 siblings, 1 reply; 51+ messages in thread
From: Joshua Otto @ 2015-12-28 5:16 UTC (permalink / raw)
To: ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
From: Chester Lin <czylin@uwaterloo.ca>
Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
is defined in IDL. This change adds an explicit cast to fix the
Coverity warning, and tweaks the surrounding code to more closely
conform to the guidelines in CODING_STYLE.
No functional changes.
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
tools/libxl/libxl_psr.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3d0dc61..1677f9c 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
uint64_t cbm)
{
GC_INIT(ctx);
- int rc;
+ int rc, r;
int socketid, nr_sockets;
rc = libxl__count_physical_sockets(gc, &nr_sockets);
@@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
libxl_for_each_set_bit(socketid, *target_map) {
if (socketid >= nr_sockets)
break;
- if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+ r = xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+ socketid, cbm);
+ if (r) {
libxl__psr_cat_log_err_msg(gc, errno);
rc = ERROR_FAIL;
}
@@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
uint64_t *cbm_r)
{
GC_INIT(ctx);
- int rc = 0;
-
- if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
+ int rc, r;
+ r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+ target, cbm_r);
+ if (r) {
libxl__psr_cat_log_err_msg(gc, errno);
rc = ERROR_FAIL;
+ } else {
+ rc = 0;
}
GC_FREE;
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2015-12-28 5:16 ` [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm Joshua Otto
@ 2016-01-04 16:40 ` Ian Campbell
2016-01-19 5:58 ` [PATCH v2 " Chester Lin
0 siblings, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-04 16:40 UTC (permalink / raw)
To: Joshua Otto, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> From: Chester Lin <czylin@uwaterloo.ca>
>
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
I assume the values are the same by construction in the IDL? Assuming so
then it would be worth mentioning that here I think, so we know why we
thought this was a valid change.
> This change adds an explicit cast to fix the
> Coverity warning, and tweaks the surrounding code to more closely
> conform to the guidelines in CODING_STYLE.
>
> No functional changes.
>
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> ---
> tools/libxl/libxl_psr.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3d0dc61..1677f9c 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t
> domid,
> uint64_t cbm)
> {
> GC_INIT(ctx);
> - int rc;
> + int rc, r;
> int socketid, nr_sockets;
>
> rc = libxl__count_physical_sockets(gc, &nr_sockets);
> @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t
> domid,
> libxl_for_each_set_bit(socketid, *target_map) {
> if (socketid >= nr_sockets)
> break;
> - if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
> cbm)) {
> + r = xc_psr_cat_set_domain_data(ctx->xch, domid,
> (xc_psr_cat_type) type,
> + socketid, cbm);
> + if (r) {
> libxl__psr_cat_log_err_msg(gc, errno);
> rc = ERROR_FAIL;
> }
> @@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t
> domid,
> uint64_t *cbm_r)
> {
> GC_INIT(ctx);
> - int rc = 0;
> -
> - if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target,
> cbm_r)) {
> + int rc, r;
> + r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)
> type,
> + target, cbm_r);
> + if (r) {
> libxl__psr_cat_log_err_msg(gc, errno);
> rc = ERROR_FAIL;
> + } else {
> + rc = 0;
> }
>
> GC_FREE;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-04 16:40 ` Ian Campbell
@ 2016-01-19 5:58 ` Chester Lin
2016-01-19 8:34 ` Dario Faggioli
2016-01-19 14:06 ` Ian Jackson
0 siblings, 2 replies; 51+ messages in thread
From: Chester Lin @ 2016-01-19 5:58 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, Chester Lin, jtotto, hjarmstr
Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
is defined in IDL.
The two enums are deliberately identical and IDL only exists so that
libxl clients don't need to include libxc headers directly.
This change adds an explicit cast to fix the
Coverity warning, and tweaks the surrounding code to more closely
conform to the guidelines in CODING_STYLE.
No functional changes.
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
Changed commit message to say that the enums are identical
---
tools/libxl/libxl_psr.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3d0dc61..1677f9c 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
uint64_t cbm)
{
GC_INIT(ctx);
- int rc;
+ int rc, r;
int socketid, nr_sockets;
rc = libxl__count_physical_sockets(gc, &nr_sockets);
@@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
libxl_for_each_set_bit(socketid, *target_map) {
if (socketid >= nr_sockets)
break;
- if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+ r = xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+ socketid, cbm);
+ if (r) {
libxl__psr_cat_log_err_msg(gc, errno);
rc = ERROR_FAIL;
}
@@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
uint64_t *cbm_r)
{
GC_INIT(ctx);
- int rc = 0;
-
- if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
+ int rc, r;
+ r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+ target, cbm_r);
+ if (r) {
libxl__psr_cat_log_err_msg(gc, errno);
rc = ERROR_FAIL;
+ } else {
+ rc = 0;
}
GC_FREE;
--
1.9.5.msysgit.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 5:58 ` [PATCH v2 " Chester Lin
@ 2016-01-19 8:34 ` Dario Faggioli
2016-01-19 14:06 ` Ian Jackson
1 sibling, 0 replies; 51+ messages in thread
From: Dario Faggioli @ 2016-01-19 8:34 UTC (permalink / raw)
To: Chester Lin, xen-devel
Cc: ian.campbell, stefano.stabellini, george.dunlap, ian.jackson,
jtotto, hjarmstr
[-- Attachment #1.1: Type: text/plain, Size: 1546 bytes --]
On Tue, 2016-01-19 at 00:58 -0500, Chester Lin wrote:
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
>
> The two enums are deliberately identical and IDL only exists so that
> libxl clients don't need to include libxc headers directly.
>
I see...
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx,
> uint32_t domid,
> libxl_for_each_set_bit(socketid, *target_map) {
> if (socketid >= nr_sockets)
> break;
> - if (xc_psr_cat_set_domain_data(ctx->xch, domid, type,
> socketid, cbm)) {
> + r = xc_psr_cat_set_domain_data(ctx->xch, domid,
> (xc_psr_cat_type) type,
> + socketid, cbm);
> + if (r) {
>
Is the cast in the function call better than a local variable of
xc_psr_cat_type initialized with 'type'? Or would Coverity keep
complaining in such a case?
If yes to either of the questions, this patch is:
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 5:58 ` [PATCH v2 " Chester Lin
2016-01-19 8:34 ` Dario Faggioli
@ 2016-01-19 14:06 ` Ian Jackson
2016-01-19 14:21 ` Ian Campbell
2016-01-19 14:31 ` Ian Campbell
1 sibling, 2 replies; 51+ messages in thread
From: Ian Jackson @ 2016-01-19 14:06 UTC (permalink / raw)
To: Chester Lin
Cc: ian.campbell, stefano.stabellini, george.dunlap, dario.faggioli,
xen-devel, jtotto, hjarmstr
Chester Lin writes ("[PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
>
> The two enums are deliberately identical and IDL only exists so that
> libxl clients don't need to include libxc headers directly.
>
> This change adds an explicit cast to fix the
> Coverity warning, and tweaks the surrounding code to more closely
> conform to the guidelines in CODING_STYLE.
I can see why Coverity is complaining. I think, overall, that the
existing situation is not really desirable.
In fact there are not two but *three* of these enums:
* XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
* enum xc_psr_cat_type (xenctrl.h)
* Enumeration("psr_cbm_type",...) (libxl_types.idl)
xc_psr.c explicitly converts between the first two with a switch
statement. libxl does no conversion.
I think our general rule is that enums from the hypervisor public
headers are OK to expose to libxl users, because that avoids a pile of
needless translation. Ian, Wei, do you agree ?
Of course in this particular case, we shouldn't expect libxl users to
consume XEN_DOMCTL_*. Instead, I would have expected
XEN_DOMCTL_PSR_CAT_OP_SET with a separate enum
XEN_PSR_CAT_L3_* or something.
With the current setup there is no mechanism (computer- or
human-mediated) that checks that new values added to these enums
correspond. And there is not even a comment that the values of the
libxl enum and the libxc enum need to be kept in step.
I am not a fan of the cast as a solution. I would rather, prefer to
regularise the situation. If my co-maintainers agree about the
desirability of expecting libxl callers to use enum values from Xen
public headers, then I would want to:
* Change the hypervisor interface
* Abolish the libxc and libxl enums
* Provide a compatibility layer in libxl for users of the old
enum value names and the old type names (do we need to keep
the old enum in the IDL or does our API stability guarantee apply
to the generated C bindings?)
Ian.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:06 ` Ian Jackson
@ 2016-01-19 14:21 ` Ian Campbell
2016-01-19 14:28 ` Dario Faggioli
2016-01-19 14:31 ` George Dunlap
2016-01-19 14:31 ` Ian Campbell
1 sibling, 2 replies; 51+ messages in thread
From: Ian Campbell @ 2016-01-19 14:21 UTC (permalink / raw)
To: Ian Jackson, Chester Lin
Cc: stefano.stabellini, george.dunlap, dario.faggioli, xen-devel,
jtotto, hjarmstr
On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> I am not a fan of the cast as a solution. I would rather, prefer to
> regularise the situation. If my co-maintainers agree about the
> desirability of expecting libxl callers to use enum values from Xen
> public headers,
libxl_shutdown_reason has the same issues, libxl_tsc_mode also might, as
might libxl_timer_mode. I'm not sure if there are others. I think we
generally handle all these the way psr is handled today (with casts and/or
explicit conversion switches).
I think part of the problem is that it is hard to expose just the desired
bits through to the user of libxl without also exposing the full xen
hypercall interface (the vast majority of which would be inappropriate to
expose to them since libxl is suppose to encapsulate such things).
If we can find a way to allow this without that downside and while
preserving API compat that would be ok.
> then I would want to:
>
> * Change the hypervisor interface
> * Abolish the libxc and libxl enums
> * Provide a compatibility layer in libxl for users of the old
> enum value names and the old type names (do we need to keep
> the old enum in the IDL or does our API stability guarantee apply
> to the generated C bindings?)
I think we do need to keep the compat, yes, the fact that bit of the C API
is autogenerated makes no difference to the end user. The compat API is
only needed for callers who define LIBXL_API_VERSION to some older version
though (I think, do check the libxl.h comments in case I'm misremembering
what we say).
As a possible alternative, we could make it such that the IDL generator
knows about the linkage and enforces the use of the same values, and
automatically provides conversion helpers (essential a cast wrapped in some
syntax) for _internal_ use.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:21 ` Ian Campbell
@ 2016-01-19 14:28 ` Dario Faggioli
2016-01-19 14:33 ` Ian Jackson
2016-01-19 14:31 ` George Dunlap
1 sibling, 1 reply; 51+ messages in thread
From: Dario Faggioli @ 2016-01-19 14:28 UTC (permalink / raw)
To: Ian Campbell, Ian Jackson, Chester Lin
Cc: george.dunlap, jtotto, stefano.stabellini, hjarmstr, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]
On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote:
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > I am not a fan of the cast as a solution. I would rather, prefer
> > to
> > regularise the situation. If my co-maintainers agree about the
> > desirability of expecting libxl callers to use enum values from Xen
> > public headers,
>
> libxl_shutdown_reason has the same issues, libxl_tsc_mode also might,
> as
> might libxl_timer_mode. I'm not sure if there are others.
>
(FTR) libxl_scheduler too, probably?
> I think we
> generally handle all these the way psr is handled today (with casts
> and/or
> explicit conversion switches).
>
and in fact, libxl_scheduler --touched in another patch of this
series-- is also handled in a similar way, and in that case, you seemed
to be fine with it? (and I'm not complaining, I'm just genuinely
confused :-) )
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:28 ` Dario Faggioli
@ 2016-01-19 14:33 ` Ian Jackson
0 siblings, 0 replies; 51+ messages in thread
From: Ian Jackson @ 2016-01-19 14:33 UTC (permalink / raw)
To: Dario Faggioli
Cc: Ian Campbell, stefano.stabellini, george.dunlap, Chester Lin,
xen-devel, jtotto, hjarmstr
Dario Faggioli writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote:
> > libxl_shutdown_reason has the same issues, libxl_tsc_mode also might,
> > as might libxl_timer_mode. I'm not sure if there are others.
> >
> (FTR) libxl_scheduler too, probably?
Quite possibly. Hrm.
I see that for shutdown_reason Ian C did this deliberately in "libxl:
Remove xen/sched.h from public interface".
I am really not a fan of this approach. I don't remember what I said
about that at the time. The result is multiple enums with the same
values in that need to be kept in step.
However, I'm not sure I can persuade everyone to agree on an
alternative. At the very least we should document the rules in
CODING_STYLE.
Ian.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:21 ` Ian Campbell
2016-01-19 14:28 ` Dario Faggioli
@ 2016-01-19 14:31 ` George Dunlap
1 sibling, 0 replies; 51+ messages in thread
From: George Dunlap @ 2016-01-19 14:31 UTC (permalink / raw)
To: Ian Campbell, Ian Jackson, Chester Lin
Cc: stefano.stabellini, george.dunlap, dario.faggioli, xen-devel,
jtotto, hjarmstr
On 19/01/16 14:21, Ian Campbell wrote:
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
>> I am not a fan of the cast as a solution. I would rather, prefer to
>> regularise the situation. If my co-maintainers agree about the
>> desirability of expecting libxl callers to use enum values from Xen
>> public headers,
<snip>
> I think part of the problem is that it is hard to expose just the desired
> bits through to the user of libxl without also exposing the full xen
> hypercall interface (the vast majority of which would be inappropriate to
> expose to them since libxl is suppose to encapsulate such things).
I agree with this sentiment...
> As a possible alternative, we could make it such that the IDL generator
> knows about the linkage and enforces the use of the same values, and
> automatically provides conversion helpers (essential a cast wrapped in some
> syntax) for _internal_ use.
...and using the IDL to enforce or generate values was my first thought.
-George
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:06 ` Ian Jackson
2016-01-19 14:21 ` Ian Campbell
@ 2016-01-19 14:31 ` Ian Campbell
2016-01-19 14:35 ` Ian Jackson
1 sibling, 1 reply; 51+ messages in thread
From: Ian Campbell @ 2016-01-19 14:31 UTC (permalink / raw)
To: Ian Jackson, Chester Lin
Cc: stefano.stabellini, george.dunlap, dario.faggioli, xen-devel,
jtotto, hjarmstr
On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> Chester Lin writes ("[PATCH v2 5/5] libxl: Add explicit cast to
> libxl_psr_cat_set_cbm"):
> > Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> > expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> > is defined in IDL.
> >
> > The two enums are deliberately identical and IDL only exists so that
> > libxl clients don't need to include libxc headers directly.
> >
> > This change adds an explicit cast to fix the
> > Coverity warning, and tweaks the surrounding code to more closely
> > conform to the guidelines in CODING_STYLE.
>
> I can see why Coverity is complaining. I think, overall, that the
> existing situation is not really desirable.
>
>
> In fact there are not two but *three* of these enums:
>
> * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> * enum xc_psr_cat_type (xenctrl.h)
> * Enumeration("psr_cbm_type",...) (libxl_types.idl)
Forgot to say in my other reply, but we could try and abolish at least the
xc one and have libxl internally use the domctl values.
I should also have said I think all of this is a bit much to ask Chester to
tackle given that the intro[0] explains that the Coverity stuff is just in
order to gain some familiarity before embarking on a "proper" project.
Ian.
[0] http://lists.xen.org/archives/html/xen-devel/2015-12/msg00629.html
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:31 ` Ian Campbell
@ 2016-01-19 14:35 ` Ian Jackson
2017-01-12 18:08 ` George Dunlap
0 siblings, 1 reply; 51+ messages in thread
From: Ian Jackson @ 2016-01-19 14:35 UTC (permalink / raw)
To: Ian Campbell
Cc: stefano.stabellini, george.dunlap, dario.faggioli, Chester Lin,
xen-devel, jtotto, hjarmstr
Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> > * enum xc_psr_cat_type (xenctrl.h)
> > * Enumeration("psr_cbm_type",...) (libxl_types.idl)
>
> Forgot to say in my other reply, but we could try and abolish at least the
> xc one and have libxl internally use the domctl values.
Yes.
I like George's IDL suggestion.
> I should also have said I think all of this is a bit much to ask Chester to
> tackle given that the intro[0] explains that the Coverity stuff is just in
> order to gain some familiarity before embarking on a "proper" project.
Well, quite! Sorry to Chester for having suddenly revealed this
apparently-simple bug to be a swamp.
Ian.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2016-01-19 14:35 ` Ian Jackson
@ 2017-01-12 18:08 ` George Dunlap
2017-01-13 9:05 ` Dario Faggioli
0 siblings, 1 reply; 51+ messages in thread
From: George Dunlap @ 2017-01-12 18:08 UTC (permalink / raw)
To: Ian Jackson
Cc: Ian Campbell, Stefano Stabellini, Dario Faggioli, Chester Lin,
xen-devel, jtotto, hjarmstr
On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
>> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
>> > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
>> > * enum xc_psr_cat_type (xenctrl.h)
>> > * Enumeration("psr_cbm_type",...) (libxl_types.idl)
>>
>> Forgot to say in my other reply, but we could try and abolish at least the
>> xc one and have libxl internally use the domctl values.
>
> Yes.
>
> I like George's IDL suggestion.
Out of curiosity, did anything ever come of this? If not it seems
like we should write it down somewhere.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm
2017-01-12 18:08 ` George Dunlap
@ 2017-01-13 9:05 ` Dario Faggioli
0 siblings, 0 replies; 51+ messages in thread
From: Dario Faggioli @ 2017-01-13 9:05 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Ian Campbell, Stefano Stabellini, Chester Lin, xen-devel, jtotto,
hjarmstr
[-- Attachment #1.1: Type: text/plain, Size: 1180 bytes --]
On Thu, 2017-01-12 at 18:08 +0000, George Dunlap wrote:
> On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote:
> >
> > > On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > > >
> > > > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> > > > * enum xc_psr_cat_type (xenctrl.h)
> > > > * Enumeration("psr_cbm_type",...) (libxl_types.idl)
> > >
> > > Forgot to say in my other reply, but we could try and abolish at
> > > least the
> > > xc one and have libxl internally use the domctl values.
> >
> > Yes.
> >
> > I like George's IDL suggestion.
>
> Out of curiosity, did anything ever come of this?
>
AFAICT, no. The patch with the casts was not taken, and we still have
all the three enums/types in Xen, libxc and libxl.
> If not it seems
> like we should write it down somewhere.
>
Indeed, especially because it was a good idea.
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: Coverity tidying
2015-12-28 5:16 ` Coverity tidying Joshua Otto
` (4 preceding siblings ...)
2015-12-28 5:16 ` [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm Joshua Otto
@ 2015-12-28 9:34 ` Andrew Cooper
2016-01-01 3:14 ` [PATCH] svm: rephrase local variable use for Coverity Joshua Otto
5 siblings, 1 reply; 51+ messages in thread
From: Andrew Cooper @ 2015-12-28 9:34 UTC (permalink / raw)
To: Joshua Otto, ian.campbell, xen-devel
Cc: wei.liu2, stefano.stabellini, george.dunlap, dario.faggioli,
ian.jackson, czylin, hjarmstr
On 28/12/15 05:16, Joshua Otto wrote:
> On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
>> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
>>> On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
>>>> Cool! Just to be clear, you are looking for one project for the 3 of
>>>> you to
>>>> work on as a group (vs 3 individual projects), is that right?
>>> Yes, that's right.
>>>
>>>> It's been a while since there has been a scan run, I did one yesterday but
>>>> it is taking an unusually long time to get the results back. Hopefully
>>>> we'll have an up to date set of defects early next week and I can have a
>>>> scrobble around for some interesting ones for you guys to take a look at.
>>> That would be perfect, thanks!
>> Results are in. I've cherry-picked a few of the new issues below. I've not
>> checked carefully for false +ves.
>>
>> Not a great deal of massive thrills in there, but some one liners etc to
>> dip your toes in I guess.
> These patches address the Coverity scan issues identified below that appear to
> be actual problems. For issues that we believe to be false positives, we
> briefly explain why.
>
> We've attempted to CC maintainers according to get_maintainer.pl.
>
>> ________________________________________________________________________________________________________
>> *** CID 1343310: Code maintainability issues (UNUSED_VALUE)
>> /xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
>> 89 struct vmcb_struct *gvmcb = nv->nv_vvmcx;
>> 90
>> 91 /* check if l1 guest injects interrupt into l2 guest via vintr.
>> 92 * return here or l2 guest looses interrupts, otherwise.
>> 93 */
>> 94 ASSERT(gvmcb != NULL);
>>>>> CID 1343310: Code maintainability issues (UNUSED_VALUE)
>>>>> Assigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that stored value is overwritten before it can be used.
>> 95 intr = vmcb_get_vintr(gvmcb);
>> 96 if ( intr.fields.irq )
>> 97 return;
>> 98 }
>> 99 }
>> 100
> intr is used on the next line, so this appears to be a false positive without an
> obvious rephrasing that Coverity would accept.
The error message isn't fantastic, but the complaint that Coverity has
is that we store intr here, then unilaterally store it again slightly
lower in the function, no matter what value it had (with the early
return presumably not being taken into account).
The error would probably be resolved if lines 95 and 96 turned into "if
( vmcb_get_vintr(gvmcb).fields.irq )"
>
>> ________________________________________________________________________________________________________
>> *** CID 1343309: Control flow issues (UNREACHABLE)
>> /tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
>> 5569 {
>> 5570 libxl_scheduler sched, ret;
>> 5571 GC_INIT(ctx);
>> 5572 if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
>> 5573 LOGE(ERROR, "getting domain info list");
>> 5574 return ERROR_FAIL;
>>>>> CID 1343309: Control flow issues (UNREACHABLE)
>>>>> This code cannot be reached: "libxl__free_all(gc);".
>> 5575 GC_FREE;
>> 5576 }
>> 5577 GC_FREE;
>> 5578 return sched;
>> 5579 }
>> 5580
>>
>> As well as putting GC_FREE in the right place this function could be
>> reworked to follow the recommendations in tools/libxl/CODING_STYLE.
> This issue is addressed by patches 1 and 2.
>
>> ** CID 1343307: (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1343307: (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> 740 ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
>> 741 if (ret == ERANGE) {
>> 742 buf_size += 128;
>> 743 continue;
>> 744 }
>> 745 if (ret != 0)
>>>>> CID 1343307: (RESOURCE_LEAK)
>>>>> Variable "buf" going out of scope leaks the storage it points to.
>> 746 return ERROR_FAIL;
>> 747 if (user != NULL)
>> 748 return 1;
>> 749 return 0;
>> 750 }
>> 751 }
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> 742 buf_size += 128;
>> 743 continue;
>> 744 }
>> 745 if (ret != 0)
>> 746 return ERROR_FAIL;
>> 747 if (user != NULL)
>>>>> CID 1343307: (RESOURCE_LEAK)
>>>>> Variable "buf" going out of scope leaks the storage it points to.
>> 748 return 1;
>> 749 return 0;
>> 750 }
>> 751 }
>> 752
>> 753 static int libxl__build_device_model_args_new(libxl__gc *gc,
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>> 743 continue;
>> 744 }
>> 745 if (ret != 0)
>> 746 return ERROR_FAIL;
>> 747 if (user != NULL)
>> 748 return 1;
>>>>> CID 1343307: (RESOURCE_LEAK)
>>>>> Variable "buf" going out of scope leaks the storage it points to.
>> 749 return 0;
>> 750 }
>> 751 }
>> 752
>> 753 static int libxl__build_device_model_args_new(libxl__gc *gc,
>> 754 const char *dm, int guest_domid,
> This appears to be a false positive - libxl__realloc() ensures that any
> new allocations are added to the gc, and that subsequent reallocations
> bring the gc up to date, so exiting the function at any time should be
> safe.
Correct. Coverity is unable to track object ownership information when
you start playing containerof() games to put it in a list.
~Andrew
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH] svm: rephrase local variable use for Coverity.
2015-12-28 9:34 ` Coverity tidying Andrew Cooper
@ 2016-01-01 3:14 ` Joshua Otto
2016-01-06 13:24 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Joshua Otto @ 2016-01-01 3:14 UTC (permalink / raw)
To: andrew.cooper3, ian.campbell, xen-devel; +Cc: Joshua Otto, hjarmstr, czylin
Coverity CID 1343310
No functional changes.
Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
> The error message isn't fantastic, but the complaint that Coverity
> has is that we store intr here, then unilaterally store it again
> slightly lower in the function, no matter what value it had (with
> the early return presumably not being taken into account).
>
> The error would probably be resolved if lines 95 and 96 turned into
> "if ( vmcb_get_vintr(gvmcb).fields.irq )"
This patch implements that change - as a general rule, is maintainer
preference to resolve false positives like this by suppressing them in
the tool or through code changes like this one?
xen/arch/x86/hvm/svm/intr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index bd94731..240eb35 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
* return here or l2 guest looses interrupts, otherwise.
*/
ASSERT(gvmcb != NULL);
- intr = vmcb_get_vintr(gvmcb);
- if ( intr.fields.irq )
+ if ( vmcb_get_vintr(gvmcb).fields.irq )
return;
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH] svm: rephrase local variable use for Coverity.
2016-01-01 3:14 ` [PATCH] svm: rephrase local variable use for Coverity Joshua Otto
@ 2016-01-06 13:24 ` Jan Beulich
2016-01-06 14:33 ` Boris Ostrovsky
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-01-06 13:24 UTC (permalink / raw)
To: Joshua Otto
Cc: ian.campbell, andrew.cooper3, czylin, xen-devel,
Aravind Gopalakrishnan, Suravee Suthikulpanit, Boris Ostrovsky,
hjarmstr
>>> On 01.01.16 at 04:14, <jtotto@uwaterloo.ca> wrote:
> Coverity CID 1343310
>
> No functional changes.
>
> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
> ---
> On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
>> The error message isn't fantastic, but the complaint that Coverity
>> has is that we store intr here, then unilaterally store it again
>> slightly lower in the function, no matter what value it had (with
>> the early return presumably not being taken into account).
>>
>> The error would probably be resolved if lines 95 and 96 turned into
>> "if ( vmcb_get_vintr(gvmcb).fields.irq )"
>
> This patch implements that change - as a general rule, is maintainer
> preference to resolve false positives like this by suppressing them in
> the tool or through code changes like this one?
Asking such a question it would be helpful if you included the
maintainers of the code in question, since to a good part this
is a matter of taste, especially when ...
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
> * return here or l2 guest looses interrupts, otherwise.
> */
> ASSERT(gvmcb != NULL);
> - intr = vmcb_get_vintr(gvmcb);
> - if ( intr.fields.irq )
> + if ( vmcb_get_vintr(gvmcb).fields.irq )
... some people (not me) frown upon complex expressions like the
one resulting here.
Also please note that while perhaps minor here, obvious with
the quote from an earlier mail conversation, naming the person
having suggested the change would be appropriate - if you
look for them, you'll find quite a few Suggested-by: tags in the
commit history.
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] svm: rephrase local variable use for Coverity.
2016-01-06 13:24 ` Jan Beulich
@ 2016-01-06 14:33 ` Boris Ostrovsky
0 siblings, 0 replies; 51+ messages in thread
From: Boris Ostrovsky @ 2016-01-06 14:33 UTC (permalink / raw)
To: Jan Beulich, Joshua Otto
Cc: ian.campbell, andrew.cooper3, czylin, xen-devel,
Aravind Gopalakrishnan, Suravee Suthikulpanit, hjarmstr
On 01/06/2016 08:24 AM, Jan Beulich wrote:
>>>> On 01.01.16 at 04:14, <jtotto@uwaterloo.ca> wrote:
>> Coverity CID 1343310
>>
>> No functional changes.
>>
>> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
>> ---
>> On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
>>> The error message isn't fantastic, but the complaint that Coverity
>>> has is that we store intr here, then unilaterally store it again
>>> slightly lower in the function, no matter what value it had (with
>>> the early return presumably not being taken into account).
>>>
>>> The error would probably be resolved if lines 95 and 96 turned into
>>> "if ( vmcb_get_vintr(gvmcb).fields.irq )"
>> This patch implements that change - as a general rule, is maintainer
>> preference to resolve false positives like this by suppressing them in
>> the tool or through code changes like this one?
I'd rather suppress this in the tool as I am one of those people that
Jan refers to below ;-)
However, if it's too much of a hassle then this patch would be OK.
-boris
> Asking such a question it would be helpful if you included the
> maintainers of the code in question, since to a good part this
> is a matter of taste, especially when ...
>
>> --- a/xen/arch/x86/hvm/svm/intr.c
>> +++ b/xen/arch/x86/hvm/svm/intr.c
>> @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
>> * return here or l2 guest looses interrupts, otherwise.
>> */
>> ASSERT(gvmcb != NULL);
>> - intr = vmcb_get_vintr(gvmcb);
>> - if ( intr.fields.irq )
>> + if ( vmcb_get_vintr(gvmcb).fields.irq )
> ... some people (not me) frown upon complex expressions like the
> one resulting here.
>
> Also please note that while perhaps minor here, obvious with
> the quote from an earlier mail conversation, naming the person
> having suggested the change would be appropriate - if you
> look for them, you'll find quite a few Suggested-by: tags in the
> commit history.
>
> Jan
>
^ permalink raw reply [flat|nested] 51+ messages in thread