Re: [Xen-devel] [PATCH RFC V2 01/45] xen/sched: add inline wrappers for calling per-scheduler functions

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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 <jgross@xxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel