question about the code

[Jef Driesen]

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.

> 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.

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.

> A patch that fixes this will be included in my patches later today.

I'll have a look at the patches. (It may take a few days, I'm spending the 
holidays with my family and kids.)

Is the Coverity report available somewhere?

Jef