On 5/9/19 6:32 AM, Juergen Gross wrote:
> On 08/05/2019 18:24, George Dunlap wrote:
>> On 5/6/19 7:56 AM, Juergen Gross wrote:
>>> Instead of using the SCHED_OP() macro to call the different scheduler
>>> specific functions add inline wrappers for that purpose.
>>>
>>> Signed-off-by: Juergen Gross <jgr...@suse.com>
>>
>> This seems like a great idea. One minor comment...
>>
>>> +static inline int sched_init(struct scheduler *s)
>>> +{
>>> + ASSERT(s->init);
>>> + return s->init(s);
>>> +}
>>> +
>>> +static inline void sched_deinit(struct scheduler *s)
>>> +{
>>> + ASSERT(s->deinit);
>>> + s->deinit(s);
>>> +}
>>
>> I think these would better as BUG_ON()s. These aren't hot paths, and if
>> we do somehow hit this situation in production, 1) it's safer to
>> BUG_ON() than dereferencing NULL, and 2) you'll get a more helpful error
>> message.
>
> Only for those 2 instances above? Or would you like BUG_ON() instead of
> ASSERT() in the other added inlines, too (maybe not for pick_cpu, but
> e.g. the ones in free_*) ?
Why not for pick_cpu()? It's the same basic logic -- in production, if
it *is* NULL, then you'll either crash with a segfault, or start
executing an exploit. Much better to BUG_ON().
As for the `ASSERT(!data)`, instances, I think it's the reverse: If
`data` turns out to be non-null, then you'll leak memory, but otherwise
keep working until you run out. If you make those BUG_ON()s, then
everything stops immediately. I think ASSERT() is the right thing in
those cases.
(I do have a draft of some guidelines for this sort of thing...
hopefully I'll find time to re-post them sometime in the next month or two.)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
