Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.11.2021 23:37, Andrew Cooper wrote:
> On 08/11/2021 09:04, Jan Beulich wrote:
>> On 05.11.2021 14:55, Andrew Cooper wrote:
>>> +void __init xsm_fixup_ops(struct xsm_ops *ops)
>>> +{
>>> +    /*
>>> +     * We make some simplifying assumptions about struct xsm_ops; that it 
>>> is
>>> +     * made exclusively of function pointers to non-init text.
>>> +     *
>>> +     * This allows us to walk over struct xsm_ops as if it were an array of
>>> +     * unsigned longs.
>>> +     */
>>> +    unsigned long *dst = _p(ops);
>>> +    unsigned long *src = _p(&dummy_ops);
>> I'm afraid I consider this an abuse of _p(): It hides casting when
>> that would better not be hidden (and there's then also a pointless
>> step through "unsigned long" in the casting). I suppose this is
>> also why "src" didn't end up "const unsigned long *" - with spelled
>> out casts the casting away of const might have been more noticable.
> 
> I've changed to a const pointer, but opencoding _p() wouldn't make it 
> any more likely for me to have spotted that it ought to have been const 
> to begin with.
> 
> But ultimately it comes down to neatness/clarity.  This:
> 
> unsigned long *dst = _p(ops);
> const unsigned long *src = _p(&dummy_ops);
> 
> is easier to read than this:
> 
> unsigned long *dst = (unsigned long *)ops;
> const unsigned long *src = (const unsigned long *)&dummy_ops;
> 
> Fundamentally, I can do either, but I have a preference for the one 
> which is easier to follow.

One option would be to at least make _p() cast to const void *.

Jan