Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- 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