Re: question about the code

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