Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

Hi Vladimir,

On Wed, Feb 24, 2021 at 11:44 PM Vladimir Oltean [off-list ref] wrote:

On Tue, Feb 23, 2021 at 12:20:03PM +0100, Geert Uytterhoeven wrote:

quoted

sja1105_unpack() takes a "const void *buf" as its first parameter, so
there is no need to cast away the "const" of the "buf" variable before
calling it.

Drop the cast, as it prevents the compiler performing some checks.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Thanks!

By the way, your email went straight to my spam box, I just found the
patch by mistake on patchwork.

    Why is this message in spam?
    It is in violation of Google's recommended email sender guidelines.

Yeah, sometimes Gmail can be annoying.  I recommend adding a filter
to never send emails with "PATCH" in the subject to spam.

quoted

Compile-tested only.

BTW, sja1105_packing() and packing() are really bad APIs, as the input
pointer parameters cannot be const due to the direction depending on
"op".  This means the compiler cannot do const checks.  Worse, callers
are required to cast away constness to prevent the compiler from
issueing warnings.  Please don't do this!
---

What const checks can the compiler not do?

If you have a const and a non-const buffer, and accidentally call
packing() with the two buffer pointers exchanged (this is a common
mistake), you won't get a compiler warning.
So having separate pack() and unpack() functions would be safer.
You can rename packing() to __packing() to make it clear this function
is not to be called directly without deep consideration, and have
pack() and unpack() as wrappers just calling __packing().
Of course that means callers that do need a separate "op" parameter
still need to call __packing(), but they can provide their own safer
wrappers, too.

Also, if you know of an existing kernel API which can replace packing(),
I'm all ears.

No idea.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds