On 6/18/21 7:44 AM, Jan Beulich wrote:
> On 18.06.2021 13:34, Andrew Cooper wrote:
>> On 18/06/2021 00:39, Daniel P. Smith wrote:
>>> @@ -197,16 +204,21 @@ bool __init has_xsm_magic(paddr_t start)
>>>
>>> int __init register_xsm(struct xsm_operations *ops)
>>> {
>>> - if ( verify(ops) )
>>> + if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
>>> + return -EAGAIN;
>>
>> I know you moved this around the function, but it really isn't -EAGAIN
>> material any more. It's "too late - nope".
>>
>> -EEXIST is probably best for "I'm never going to tolerate another call".
>>
>>> +
>>> + if ( !ops )
>>> {
>>> - printk(XENLOG_ERR "Could not verify xsm_operations structure\n");
>>> + xsm_ops_registered = XSM_OPS_REG_FAILED;
>>> + printk(XENLOG_ERR "Invalid xsm_operations structure registered\n");
>>> return -EINVAL;
>>
>> Honestly, I'd be half tempted to declare register_xsm() with
>> __nonnull(0) and let the compiler reject any attempt to pass a NULL ops
>> pointer.
>>
>> Both callers pass a pointer to a static singleton objects.
>
> Why check at all when the source of the arguments is all internal?
> We don't check pointers to be non-NULL elsewhere, with a few odd
> exceptions (which imo should all be dropped).
In verify(ops) there was a check on ops for NULL, I pulled that check up
into here as I removed verify(). Based on Andy's comment, this function
is looking like it is on the chopping block as well.
v/r,
dps
