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
