On Thu, Feb 21, 2013 at 05:16:29AM +0000, Katerina Barone-Adesi wrote:
Dear Katerina,
thanks for the patch. There was one issue left, I tried to fix it myself,
pushed it and only made it worse. So much for following the process. :)
Your fixed version made "log alarms 2" have be able to hold two entries
but "write terminal" was emitting "log alarms 3". So I have decided to
move the +1/-1 into the loggingrb.c. I have attempted to update the API
docs at the places. Please have another look if I have done it correctly.
For new header files one needs to add them to the include/**Makefile.am
files, otherwise a "make dist" will not package these files. This kind of
error is caught on our jenkins system (which runs make distcheck).
One thing I didn't notice during the review is the usage of assert(). We
are not checking if NDEBUG is in the CPPFLAGS/CFLAGS or not. This is why
I normally come up with my own assertion macro to make sure that the
assertion code is always enabled. It would be nice if you could come up
with a patch to do that. My hint would be to take a look at cocci to do
semantic patching.
The diff between your patch and my fixup attempt is attached to this
emails.
thanks a lot for your contribution
holger
diff --git a/.gitignore b/.gitignore
index aedd9fd..2ed0144 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,7 @@ tests/logging/logging_test
tests/fr/fr_test
tests/loggingrb/loggingrb_test
tests/ringbuf/ringbuf_test
+tests/strrb/strrb_test
utils/osmo-arfcn
utils/osmo-auc-gen
diff --git a/include/Makefile.am b/include/Makefile.am
index 60b9ea9..317968a 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -16,6 +16,7 @@ nobase_include_HEADERS = \
osmocom/core/linuxlist.h \
osmocom/core/linuxrbtree.h \
osmocom/core/logging.h \
+ osmocom/core/loggingrb.h \
osmocom/core/msgb.h \
osmocom/core/panic.h \
osmocom/core/prim.h \
@@ -25,6 +26,7 @@ nobase_include_HEADERS = \
osmocom/core/signal.h \
osmocom/core/socket.h \
osmocom/core/statistics.h \
+ osmocom/core/strrb.h \
osmocom/core/timer.h \
osmocom/core/utils.h \
osmocom/core/write_queue.h \
diff --git a/src/loggingrb.c b/src/loggingrb.c
index bb339eb..b206fde 100644
--- a/src/loggingrb.c
+++ b/src/loggingrb.c
@@ -48,14 +48,14 @@ size_t log_target_rb_used_size(struct log_target const *target)
/*! \brief Return the capacity of the osmo_strrb-backed target.
* \param[in] target The target to search.
*
- * Note that this is the capacity (aka max number of messages +1).
+ * Note that this is the capacity (aka max number of messages).
* It is not the number of unused message slots.
* \return The number of log strings in the osmo_strrb-backed target.
*/
size_t log_target_rb_avail_size(struct log_target const *target)
{
struct osmo_strrb *rb = target->tgt_rb.rb;
- return rb->size;
+ return rb->size - 1;
}
/*! \brief Return the nth log entry in a target.
@@ -70,7 +70,7 @@ const char *log_target_rb_get(struct log_target const *target, size_t logindex)
}
/*! \brief Create a new logging target for ringbuffer-backed logging.
- * \param[in] size The size of the internal backing osmo_strrb (messages +1).
+ * \param[in] size The capacity (number of messages) of the logging target.
* \returns A log target in case of success, NULL in case of error.
*/
struct log_target *log_target_create_rb(size_t size)
@@ -82,7 +82,7 @@ struct log_target *log_target_create_rb(size_t size)
if (!target)
return NULL;
- rb = osmo_strrb_create(target, size);
+ rb = osmo_strrb_create(target, size + 1);
if (!rb) {
log_target_destroy(target);
return NULL;
diff --git a/src/strrb.c b/src/strrb.c
index 580c65c..b137acb 100644
--- a/src/strrb.c
+++ b/src/strrb.c
@@ -46,10 +46,11 @@
/*! \brief Create an empty, initialized osmo_strrb.
* \param[in] ctx The talloc memory context which should own this.
- * \param[in] rb_size The number of messages the osmo_strrb can hold.
+ * \param[in] rb_size The number of message slots the osmo_strrb can hold.
* \returns A struct osmo_strrb* on success, NULL in case of error.
*
* This function creates and initializes a ringbuffer.
+ * Note that the ringbuffer stores at most rb_size - 1 messages.
*/
struct osmo_strrb *osmo_strrb_create(TALLOC_CTX * ctx, size_t rb_size)
@@ -62,7 +63,7 @@ struct osmo_strrb *osmo_strrb_create(TALLOC_CTX * ctx, size_t rb_size)
goto alloc_error;
/* start and end are zero already, which is correct */
- rb->size = rb_size + 1; /* one space is overhead */
+ rb->size = rb_size;
rb->buffer = talloc_array(rb, char *, rb->size);
if (!rb->buffer)
diff --git a/tests/strrb/strrb_test.c b/tests/strrb/strrb_test.c
index b7c5e27..abe649f 100644
--- a/tests/strrb/strrb_test.c
+++ b/tests/strrb/strrb_test.c
@@ -33,7 +33,7 @@ struct osmo_strrb *rb0, *rb1, *rb2, *rb3, *rb4, *rb5;
#define STR3 "sky"
#define STR4 "moon"
-#define TESTSIZE 2
+#define TESTSIZE 3
void init_rbs(void)
{
@@ -123,7 +123,7 @@ void test_getn_wrap(void)
void test_add(void)
{
- struct osmo_strrb *rb = osmo_strrb_create(NULL, 3);
+ struct osmo_strrb *rb = osmo_strrb_create(NULL, 4);
assert(rb->start == 0);
assert(rb->end == 0);
