| Message ID | 1451279808-25264-3-git-send-email-jtotto@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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)
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.
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;
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(-)