On 2018-01-03 20:35, Dirk Hohndel wrote:
We sometimes ignored its return value, sometimes we didn't. Semantically, clearing a non-existing buffer is just a noop.
Careful - one side effect of the old semantic was that dc_buffer_clear() plus a bail on failure could be used to test if the buffer had been allocated. Right now there is at least one call site where we later call dc_buffer_apend() without checking its return value. This will be fixed in a later commit in this series.
Hmm, that's something I didn't think of. And there are two other potential side effects as well, that could cause us some trouble.
Before this patch, passing a NULL pointer would be detected immediately, before sending anything to the dive computer. After this patch, there are a few cases where the check is delayed. So that means we send some request to the dive computer, but might bail out before reading the response (or only a part of it). And that can easily cause a next request to fail as well, because the response data (or a part of it) remains in the driver or OS buffers.
For errors that we can detect early, like a NULL buffer, we should bail out as soon as possible. So instead of removing the dc_buffer_clear() based check, and postponing the check to some other buffer function further down, we should check it explicitly at the start of the function:
if (buffer == NULL) { ERROR (context, "No buffer space available."); return DC_STATUS_INVALIDARGS; }
(Note that I also replaced DC_STATUS_NOMEMORY with DC_STATUS_INVALIDARGS because that's probably a more suitable error code here.)
For the dump functions, which is the bulk of this patch, we can easily address all of them at once in the dc_device_dump function:
dc_status_t dc_device_dump (dc_device_t *device, dc_buffer_t *buffer) { if (device == NULL) return DC_STATUS_UNSUPPORTED;
if (device->vtable->dump == NULL) return DC_STATUS_UNSUPPORTED;
if (buffer == NULL) return DC_STATUS_INVALIDARGS;
return device->vtable->dump (device, buffer); }
Actually, also the dc_buffer_clear() call can be moved there. We always want to start with an empty buffer.
Another concern is that the dc_buffer_clear() function is part of the public api. So this change might break some applications. I don't mind doing this particular change, because it makes sense. But at the same time we also need to be careful that a simple fix to silence coverity doesn't turns into a problem for applications. (I don't think there are many applications using this function, but I don't know for sure.)
Jef