Somebody else gets a learning opportunity
Eric S. Raymond
esr at thyrsus.com
Mon Dec 7 12:19:18 UTC 2015
Sigh. There was a refactoring change I was trying to do that I
reluctantly conclude I have put too much time into. I can't let it
distract me from finishing TESTFRAME any longer than I have.
I could just throw this patch away, but it's still a good idea. So
I'm going to toss the failure at the list (enclosed) and hope somebody
else picks it up and finishes it. Apply, run ntpd -g -n and watch it
core-dump. There's instrumentation to show the call sequence.
What I'm trying to do here is hide the allocation-management members in
struct recvbuf so only the code that allocates and frees these things can
see them. It almost works, but not quite. Looks like something in
either initialize_buffer() or add_full_recv_buffer() is stomping on the
linked-list state.
It's probably something stupid and simple, but I gotta focus. I can do
cature mode without this, though the result won't look as elegant.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
-------------- next part --------------
diff --git a/include/recvbuff.h b/include/recvbuff.h
index d0f12e7..0fa466e 100644
--- a/include/recvbuff.h
+++ b/include/recvbuff.h
@@ -47,7 +47,6 @@ extern HANDLE get_recv_buff_event(void);
typedef struct recvbuf recvbuf_t;
struct recvbuf {
- recvbuf_t * link; /* next in list */
union {
sockaddr_u X_recv_srcadr;
struct peer * X_recv_peer;
@@ -70,7 +69,6 @@ struct recvbuf {
} recv_space;
#define recv_pkt recv_space.X_recv_pkt
#define recv_buffer recv_space.X_recv_buffer
- int used; /* reference count */
};
extern void init_recvbuff(int);
diff --git a/libntp/recvbuff.c b/libntp/recvbuff.c
index 8d601fc..ccd2abe 100644
--- a/libntp/recvbuff.c
+++ b/libntp/recvbuff.c
@@ -9,6 +9,17 @@
#include "recvbuff.h"
#include "iosignal.h"
+typedef struct recvwrap recvwrap_t;
+
+struct recvwrap {
+ /*
+ * Do not reorder these members! We need to be able to cast from struct
+ * recvbuf to struct recvwrapper and get sane results.
+ */
+ struct recvbuf payload;
+ recvwrap_t *link; /* next in list */
+ int used; /* reference count */
+};
/*
* Memory allocation
@@ -20,8 +31,8 @@ static u_long volatile lowater_adds; /* number of times we have added memory */
static u_long volatile buffer_shortfall;/* number of missed free receive buffers
between replenishments */
-static DECL_FIFO_ANCHOR(recvbuf_t) full_recv_fifo;
-static recvbuf_t * free_recv_list;
+static DECL_FIFO_ANCHOR(recvwrap_t) full_recv_fifo;
+static recvwrap_t * free_recv_list;
#if defined(SYS_WINNT)
@@ -42,43 +53,51 @@ static CRITICAL_SECTION RecvLock;
static void uninit_recvbuff(void);
#endif
+#define RDEBUG(...) fprintf(stderr, __VA_ARGS__);
u_long
free_recvbuffs (void)
{
+ RDEBUG("free_recvbuffs()")
return free_recvbufs;
}
u_long
full_recvbuffs (void)
{
+ RDEBUG("full_recvbuffs()")
return full_recvbufs;
}
u_long
total_recvbuffs (void)
{
+ RDEBUG("total_recvbuffs()")
return total_recvbufs;
}
u_long
lowater_additions(void)
{
+ RDEBUG("lowater_additions()")
return lowater_adds;
}
static inline void
initialise_buffer(recvbuf_t *buff)
{
+ RDEBUG("initialize_buffer(%p)", buff)
ZERO(*buff);
}
static void
create_buffers(int nbufs)
{
- register recvbuf_t *bufp;
+ register recvwrap_t *bufp;
int i, abuf;
+ RDEBUG("create_buffers(%d)", nbufs)
+
abuf = nbufs + buffer_shortfall;
buffer_shortfall = 0;
@@ -107,6 +126,7 @@ create_buffers(int nbufs)
void
init_recvbuff(int nbufs)
{
+ RDEBUG("init_recvbuf(%d)", nbufs)
/*
* Init buffer free list and stat counters
@@ -130,8 +150,9 @@ init_recvbuff(int nbufs)
static void
uninit_recvbuff(void)
{
- recvbuf_t *rbunlinked;
+ recvwrap_t *rbunlinked;
+ RDEBUG("uninit_recvbuf()")
for (;;) {
UNLINK_FIFO(rbunlinked, full_recv_fifo, link);
if (rbunlinked == NULL)
@@ -155,16 +176,18 @@ uninit_recvbuff(void)
void
freerecvbuf(recvbuf_t *rb)
{
+ RDEBUG("freerecvbuf(%p)", rb)
if (rb == NULL) {
msyslog(LOG_ERR, "freerecvbuff received NULL buffer");
return;
}
+ recvwrap_t *rw = (recvwrap_t *)rb;
LOCK();
- rb->used--;
- if (rb->used != 0)
- msyslog(LOG_ERR, "******** freerecvbuff non-zero usage: %d *******", rb->used);
- LINK_SLIST(free_recv_list, rb, link);
+ rw->used--;
+ if (rw->used != 0)
+ msyslog(LOG_ERR, "******** freerecvbuff non-zero usage: %d *******", rw->used);
+ LINK_SLIST(free_recv_list, rw, link);
free_recvbufs++;
UNLOCK();
}
@@ -173,12 +196,14 @@ freerecvbuf(recvbuf_t *rb)
void
add_full_recv_buffer(recvbuf_t *rb)
{
+ RDEBUG("add_full_recv_buffer(%p)", rb)
if (rb == NULL) {
msyslog(LOG_ERR, "add_full_recv_buffer received NULL buffer");
return;
}
+ recvwrap_t *rw = (recvwrap_t *)rb;
LOCK();
- LINK_FIFO(full_recv_fifo, rb, link);
+ LINK_FIFO(full_recv_fifo, rw, link);
full_recvbufs++;
UNLOCK();
}
@@ -187,20 +212,22 @@ add_full_recv_buffer(recvbuf_t *rb)
recvbuf_t *
get_free_recv_buffer(void)
{
- recvbuf_t *buffer;
+ recvwrap_t *wrapper;
+ RDEBUG("get_free_recv_buffer()")
LOCK();
- UNLINK_HEAD_SLIST(buffer, free_recv_list, link);
- if (buffer != NULL) {
+ UNLINK_HEAD_SLIST(wrapper, free_recv_list, link);
+ if (wrapper != NULL) {
free_recvbufs--;
- initialise_buffer(buffer);
- buffer->used++;
+ initialise_buffer(&wrapper->payload);
+ wrapper->link = NULL;
+ wrapper->used = 1;
} else {
buffer_shortfall++;
}
UNLOCK();
- return buffer;
+ return &wrapper->payload;
}
@@ -210,6 +237,7 @@ get_free_recv_buffer_alloc(void)
{
recvbuf_t *buffer;
+ RDEBUG("get_free_recv_buffer_alloc()")
buffer = get_free_recv_buffer();
if (NULL == buffer) {
create_buffers(RECV_INC);
@@ -224,7 +252,9 @@ get_free_recv_buffer_alloc(void)
recvbuf_t *
get_full_recv_buffer(void)
{
- recvbuf_t * rbuf;
+ recvwrap_t *wrapper;
+
+ RDEBUG("get_full_recv_buffer()")
LOCK();
@@ -248,12 +278,12 @@ get_full_recv_buffer(void)
/*
* try to grab a full buffer
*/
- UNLINK_FIFO(rbuf, full_recv_fifo, link);
- if (rbuf != NULL)
+ UNLINK_FIFO(wrapper, full_recv_fifo, link);
+ if (wrapper != NULL)
full_recvbufs--;
UNLOCK();
- return rbuf;
+ return &wrapper->payload;
}
@@ -266,9 +296,11 @@ purge_recv_buffers_for_fd(
SOCKET fd
)
{
- recvbuf_t *rbufp;
- recvbuf_t *next;
- recvbuf_t *punlinked;
+ recvwrap_t *rbufp;
+ recvwrap_t *next;
+ recvwrap_t *punlinked;
+
+ RDEBUG("purge_recv_buffers_for_fd()")
LOCK();
@@ -276,12 +308,12 @@ purge_recv_buffers_for_fd(
rbufp != NULL;
rbufp = next) {
next = rbufp->link;
- if (rbufp->fd == fd) {
+ if (rbufp->payload.fd == fd) {
UNLINK_MID_FIFO(punlinked, full_recv_fifo,
- rbufp, link, recvbuf_t);
+ rbufp, link, recvwrap_t);
INSIST(punlinked == rbufp);
full_recvbufs--;
- freerecvbuf(rbufp);
+ freerecvbuf(&rbufp->payload);
}
}
@@ -294,7 +326,9 @@ purge_recv_buffers_for_fd(
*/
bool has_full_recv_buffer(void)
{
- if (HEAD_FIFO(full_recv_fifo) != NULL)
+ RDEBUG("has_full_recv_buffer()")
+
+ if (HEAD_FIFO(full_recv_fifo) != NULL)
return (true);
else
return (false);
@@ -309,6 +343,8 @@ check_gen_fifo_consistency(void *fifo)
gen_node *pthis;
gen_node **pptail;
+ RDEBUG("check_gen_fifo_consistency()")
+
pf = fifo;
REQUIRE((NULL == pf->phead && NULL == pf->pptail) ||
(NULL != pf->phead && NULL != pf->pptail));
More information about the devel
mailing list