[PATCH 09/14] Cleanup: dc_buffer_clear() should be a void function

[Jef Driesen]

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