From d16264fd82133158777d0c233353cc535045f800 Mon Sep 17 00:00:00 2001 From: Erik Boasson Date: Tue, 7 May 2019 15:12:31 +0800 Subject: [PATCH] a read/take returning no data must be a no-op In all cases where read/take allocates memory for storing samples but the result turns out to be an empty set, the (observable) state of the system should end up unchanged. It turns out several cases were/are considered: * application supplies buffers (i.e., buf[0] != NULL): no memory allocated, so no issue. * reader has no cached set ("m_loan" in the current code): read/take allocated memory, cached the address and marked it as in use ("m_loan_out"), and modified buf[0] (and subsequent entries). To undo this on returning an empty set, it now: resets the "m_loan_out" flag to allow the cached buffer to be reused, and sets buf[0] back to NULL. * reader has a cached set, but it is not marked in use: read/take marked it as in use and modified buf[0] (and subsequent entries). To undo this, it now resets "m_loan_out" to indicate the cached buffer is not in use, and sets buf[0] back to NULL. * reader has a cached set that is currently in use: read/take allocated memory and updated buf[0] (and subsequent entries) but left the cached state alone. To undo this, it now frees the memory and sets buf[0] back to NULL. With this, in any path where the application lets dds_read/dds_take allocate memory for the samples: * it can still safely call dds_return_loan with buf[0] and the actual return value of read/take (even if an error code), and whatever memory was allocated will not be leaked; * but it no longer has to do so when the result was empty (or error). Signed-off-by: Erik Boasson --- src/core/ddsc/src/dds_read.c | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/core/ddsc/src/dds_read.c b/src/core/ddsc/src/dds_read.c index 2791a44..1702a25 100644 --- a/src/core/ddsc/src/dds_read.c +++ b/src/core/ddsc/src/dds_read.c @@ -94,6 +94,10 @@ dds_read_impl( dds_retcode_t rc; struct dds_reader * rd; struct dds_readcond * cond; + unsigned nodata_cleanups = 0; +#define NC_CLEAR_LOAN_OUT 1u +#define NC_FREE_BUF 2u +#define NC_RESET_BUF 4u if (buf == NULL) { DDS_ERROR("The provided buffer is NULL\n"); @@ -140,21 +144,25 @@ dds_read_impl( /* Allocate, use or reallocate loan cached on reader */ if (rd->m_loan_out) { ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs); + nodata_cleanups = NC_FREE_BUF | NC_RESET_BUF; } else { if (rd->m_loan) { if (rd->m_loan_size < maxs) { ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, rd->m_loan, rd->m_loan_size, maxs); rd->m_loan = buf[0]; rd->m_loan_size = maxs; + nodata_cleanups = NC_RESET_BUF; } else { - buf[0] = rd->m_loan; + buf[0] = rd->m_loan; + nodata_cleanups = NC_RESET_BUF; } - } else { - ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs); - rd->m_loan = buf[0]; - rd->m_loan_size = maxs; - } + } else { + ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs); + rd->m_loan = buf[0]; + rd->m_loan_size = maxs; + } rd->m_loan_out = true; + nodata_cleanups |= NC_CLEAR_LOAN_OUT; } } @@ -169,9 +177,24 @@ dds_read_impl( ddsrt_mutex_unlock (&rd->m_entity.m_observers_lock); if (take) { - ret = (dds_return_t)dds_rhc_take(rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond); + ret = (dds_return_t) dds_rhc_take (rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond); } else { - ret = (dds_return_t)dds_rhc_read(rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond); + ret = (dds_return_t) dds_rhc_read (rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond); + } + + /* if no data read, restore the state to what it was before the call, with the sole + exception of holding on to a buffer we just allocated and that is pointed to by + rd->m_loan */ + if (ret <= 0 && nodata_cleanups) { + if (nodata_cleanups & NC_CLEAR_LOAN_OUT) { + rd->m_loan_out = false; + } + if (nodata_cleanups & NC_FREE_BUF) { + ddsi_sertopic_free_samples (rd->m_topic->m_stopic, buf[0], maxs, DDS_FREE_ALL); + } + if (nodata_cleanups & NC_RESET_BUF) { + buf[0] = NULL; + } } dds_read_unlock(rd, cond);