Re: [PATCH 1/6] xsm: refactor xsm_ops handling

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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