Got a sample you think is particularly bad?

▲ irishcoffee 7 hours ago | parent [-]

Throwing myself to the sharks here, but sure.

pcie.c

  /*
  * BAR0 register access
  */
  uint32_t
  brcmf_reg_read(struct brcmf_softc *sc, uint32_t off)
  {
 return (bus_space_read_4(sc->reg_bst, sc->reg_bsh, off));
  }

Is sc null? Who knows! Was it memset anywhere? No! Were any structs memset anywhere? Barely! Does this codebase check for null? Maybe in 3% of the places it should!

All throughout this codebase variables are declared and not initialized. Magic numbers are everywhere AND constants are defined everywhere. Constants are a mix of hex and int for what seem to be completely arbitrary reasons. Error handling is completely inconsistent, sometimes a function will return 5 places, sometimes a function will set an error code and jump to a label, and sometimes do both in the same function depending on which branch it hits.

All of this is the kind of code smell I would ask someone to justify and most likely rework.

Or I'm just a dumbass, I suppose I'll find out shortly.

	▲	jeremyjh 3 hours ago \| parent [-]
		I'm no expert either and I have not done kernel development, but I've done some embedded stuff in C and I think this is not unreasonable. brcmf_reg_read is only called in one place and the call chain is straightforward (starts in pcie.c brcmf_pcie_attach). Its always initialized by device_get_softc (https://man.freebsd.org/cgi/man.cgi?query=device_get_softc&s...) so as long as the device is attached its initialized. Likely something fails much earlier if it is not attached. I think this is pretty typical for low-level C, it would definitely not be idiomatic for every function in this call chain to check if sc was initialized - I don't know if there is a need to check it after calling device_get_softc but that would probably be reasonable just so people reviewing it don't have to check. Some application code bases I've worked in would have asserts in every function since they get removed in release builds but I don't know that debug builds are even a thing in kernel devices.