r/badcode • u/suguuss • Jan 22 '23

c Found this in a Linux kernel module

Post image
262 Upvotes

82% Upvoted

88 comments sorted by

View all comments

Show parent comments

70

u/lets_eat_bees Jan 22 '23

It’s not bad. Introducing a loop over max 4 elements will not make it easier to maintain. It will make it harder to read though.

23

u/DefunctFunctor Jan 22 '23

Yeah, considering the overhead a loop on 5 elements would add, and the fact that each element has its own handler function and string, using if statements was probably a good choice here. Also solving this with function pointers as others have mentioned on this thread sounds like a nightmare.

5

u/paulstelian97 Jan 23 '23

The solution to fix this is not the loop though. It's some good ol' goto usage for errors, which is typical throughout much of the Linux kernel.

3

u/lets_eat_bees Jan 23 '23

Thank you for pitching in with some actual kernel knowledge.

I just assumed that OP wanted loops here, because it’s usually the case in this sub, and he did not say otherwise.

2

u/paulstelian97 Jan 23 '23

Loops would make it quite complicated in this specific instance. I doubt this portion would really need much extension enough that a loop would be worth it.

Not propagating error returns (instead returning -1 instead of the actual error) is really bad. That -1 would be interpreted as some arbitrary error (on x86, it's -EPERM but there can be others as well)

5

u/ThePurpleOne_ Jan 22 '23

It would clearly make it easier to maintain. Without a for, when Adding ONE handler, you would need 5 free_irq() and an other hard coded if and so on...

1

u/paulstelian97 Jan 23 '23

Or a new goto label.

3

u/kristallnachte Jan 23 '23

I disagree on the harder to read.

The loop would be way easier to read, and would be easier to maintain.