CodeSOD: The Device Search

I started writing a paragraph about why this code Gilda found was bad, and then I had to delete it all, because I wasn't putting the entire block in context. At a glance, this looks almost fine, but I thought I spotted a WTF. But only when I thought about the fact that this C code runs inside of a loop that I realized the real problem.

rsts = get_device_by_id ( movq_p->nxt_device_id, &devc );if ( ( rsts == CC_VL_SUCCESS ) && ( strcmp ( devc.device_type, SPECIFIC_DEVICE ) == 0 ) ){ specific_device_flag = CC_VL_TRUE;}/* *Process device... */if ( specific_device_flag ){...}

So, inside of a loop, this iterates across a series of devices, represented by their nxt_device_id. They load that into a device struct, devc, and do some validation on the type of device in question. If the type of device is SPECIFIC_DEVICE, then we set a flag to represent that. Later in the code, we have special processing if it's that SPECIFIC_DEVICE.

The problem here is that this code runs inside a loop and specific_device_flag is never set to false. So as we iterate across the devices, if one of them is a SPECIFIC_DEVICE, every future device will also be treated as if it's a SPECIFIC_DEVICE.

Gilda writes: "Apparently this has been in the baseline code since before the project it is in was branched off so I don't know if anything was deleted between setting the specific_device_flag and testing for it."

The beauty of this bug is that depending on the order of the device enumeration, or the number of connected devices, it might never be seen. In fact, that's been mostly the case for Gilda's company. There have been a number of tickets resolved by "try unplugging all the devices and plugging them back in to different ports" or just "reboot the system". No one knew why.

My kingdom for an else clause. Or just a boolean assignment expression. Or, if you really want to use CC_VL_TRUE and not just "non-zero is true", a ternary might actually be more readable.

I've read C programming styleguides that require every if to have an else, and if the else is empty, a comment justifying its emptiness. I usually think that's overkill, but this code sample is a strong argument in favor of such a guideline.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.