Hi Jef,
On Dec 28, 2017, at 11:43 PM, Jef Driesen jef@libdivecomputer.org wrote:
On 28-12-17 19:16, Dirk Hohndel wrote:
I'm in the middle of addressing some issues that static code analysis has uncovered in the libdivecomputer code.
I use clang's static analyzer on a regular base, so I'm surprised Coverity found quite a few more issues.
Coverity tends to be a lot better than the open source analyzers. Thankfully, they allow free use for open source projects. Lately we have started to include the Subsurface-branch of libdivecomputer with our scans and that's how those issues were found.
One of the things pointed out by Coverity is that this piece of code is pretty non-sensical (uwatec_smart.c around line 222): static dc_status_t uwatec_smart_device_close (dc_device_t *abstract) { dc_status_t status = DC_STATUS_SUCCESS; uwatec_smart_device_t *device = (uwatec_smart_device_t*) abstract; dc_status_t rc = DC_STATUS_SUCCESS; // Close the device. rc = dc_iostream_close (device->iostream); if (status != DC_STATUS_SUCCESS) { dc_status_set_error(&status, rc); } return status; } So we set status to DC_STATUS_SUCCESS Then we set rc to DC_STATUS_SUCCESS Then we immediately overwrite rc with the return value of dc_iostream_close Then we test status(!) for being different from DC_STATUS_SUCCESS (which of course it never is as we explicitly set it to that value). Inside that we call a helper function that overwites status, but only if it is not equal to DC_STATUS_SUCCESS And then we return - ta-da! - status. What I /think/ you wanted to write was this: static dc_status_t uwatec_smart_device_close (dc_device_t *abstract) { uwatec_smart_device_t *device = (uwatec_smart_device_t*) abstract; // Close the device and pass up the return code return dc_iostream_close (device->iostream); } Am I missing something here?
No, you found a bug there! The intention was to check rc (and not status) in the if statement. With that error fixed, the code should be correct.
With that fixed the code is... err, slightly over-complicated, but it does fundamentally do the same thing as my version :-)
The construct with the dc_status_set_error() call is mainly intended for cases where multiple resources needs to be released. In those cases, I want to return the error code of the first error, because that's usually the most interesting one. See for example cressi_edy_device_close and others.
You are right that if there is just one resource to free, the whole function can be simplified. I just used the same code everywhere (mostly copy and paste), but I don't mind going for the shorter version.
You have that in one of the 14 patches that I sent yesterday.
Is the Coverity report available somewhere?
For open source projects the reports are public: https://scan.coverity.com/projects/subsurface-divelog-subsurface?tab=overvie... - when I just tried this it looks like there's a step where you need an account and then need me to add you to the project (which seems counter intuitive since I've set the reports to be public...). Simply log in with your GitHub account and click the "Add me to project" button.
The latest report has the patches that I sent applied already (and it shows that I missed one issue). But in the main menu you can look at older reports and the one from yesterday still shows all the reports on libdivecomputer. Also, once you are added to the project, you can directly look up the defects by the CID# that I included in each commit.
/D