| Message ID | 149270928606.1751.8172963085482513292.stgit@nfdev2.cica.es |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Thu, Apr 20, 2017 at 07:28:06PM +0200, Arturo Borrero Gonzalez wrote: > These warnings, if they happen, should help users. > > Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org> > --- > src/channel.c | 6 +++++- > src/queue_tx.c | 11 +++++++++-- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/channel.c b/src/channel.c > index acbfa7d..b2f114d 100644 > --- a/src/channel.c > +++ b/src/channel.c > @@ -19,6 +19,7 @@ > #include "channel.h" > #include "network.h" > #include "queue.h" > +#include "log.h" > > static struct channel_ops *ops[CHANNEL_MAX]; > extern struct channel_ops channel_mcast; > @@ -161,8 +162,11 @@ static void channel_enqueue_errors(struct channel *c) > struct channel_error *error; > > qobj = queue_object_new(Q_ELEM_ERR, sizeof(struct channel_error)); > - if (qobj == NULL) > + if (qobj == NULL) { > + dlog(LOG_WARNING, "could not enqueue channel errors, failed to" > + " allocate memory"); Did you ever hit this? Moreover, we have stats that can be dumped via option. Better use them there? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 April 2017 at 13:34, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Thu, Apr 20, 2017 at 07:28:06PM +0200, Arturo Borrero Gonzalez wrote: >> These warnings, if they happen, should help users. >> >> Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org> >> --- >> src/channel.c | 6 +++++- >> src/queue_tx.c | 11 +++++++++-- >> 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/src/channel.c b/src/channel.c >> index acbfa7d..b2f114d 100644 >> --- a/src/channel.c >> +++ b/src/channel.c >> @@ -19,6 +19,7 @@ >> #include "channel.h" >> #include "network.h" >> #include "queue.h" >> +#include "log.h" >> >> static struct channel_ops *ops[CHANNEL_MAX]; >> extern struct channel_ops channel_mcast; >> @@ -161,8 +162,11 @@ static void channel_enqueue_errors(struct channel *c) >> struct channel_error *error; >> >> qobj = queue_object_new(Q_ELEM_ERR, sizeof(struct channel_error)); >> - if (qobj == NULL) >> + if (qobj == NULL) { >> + dlog(LOG_WARNING, "could not enqueue channel errors, failed to" >> + " allocate memory"); > > Did you ever hit this? > I don't know, no way to know in a production system since this happen silently. Since conntrackd can be of critical importance in some environments I guess it doesn't harm to be more verbose. This concrete memory allocation failure isn't interesting per se, but it could be related to other more serious issues on the system. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 25, 2017 at 02:40:45PM +0200, Arturo Borrero Gonzalez wrote: > On 25 April 2017 at 13:34, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Thu, Apr 20, 2017 at 07:28:06PM +0200, Arturo Borrero Gonzalez wrote: > >> These warnings, if they happen, should help users. > >> > >> Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org> > >> --- > >> src/channel.c | 6 +++++- > >> src/queue_tx.c | 11 +++++++++-- > >> 2 files changed, 14 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/channel.c b/src/channel.c > >> index acbfa7d..b2f114d 100644 > >> --- a/src/channel.c > >> +++ b/src/channel.c > >> @@ -19,6 +19,7 @@ > >> #include "channel.h" > >> #include "network.h" > >> #include "queue.h" > >> +#include "log.h" > >> > >> static struct channel_ops *ops[CHANNEL_MAX]; > >> extern struct channel_ops channel_mcast; > >> @@ -161,8 +162,11 @@ static void channel_enqueue_errors(struct channel *c) > >> struct channel_error *error; > >> > >> qobj = queue_object_new(Q_ELEM_ERR, sizeof(struct channel_error)); > >> - if (qobj == NULL) > >> + if (qobj == NULL) { > >> + dlog(LOG_WARNING, "could not enqueue channel errors, failed to" > >> + " allocate memory"); > > > > Did you ever hit this? > > > > I don't know, no way to know in a production system since this happen silently. No problem. I just wanted to know if you're addressing a real issue or you just found this spot with not log message when passing by. > Since conntrackd can be of critical importance in some environments I > guess it doesn't harm to be more verbose. This concrete memory > allocation failure isn't interesting per se, but it could be related > to other more serious issues on the system. Yes, but this is going to full the logs if ever happen. Better add stats: /* statistics */ struct { uint64_t msg_rcv_malformed; uint32_t msg_rcv_bad_version; uint32_t msg_rcv_bad_payload; uint32_t msg_rcv_bad_header; uint32_t msg_rcv_bad_type; uint32_t msg_rcv_truncated; uint32_t msg_rcv_bad_size; uint32_t msg_snd_malformed; uint64_t msg_rcv_lost; uint64_t msg_rcv_before; } error; A quick glance at the code to see how we're globaling deal with lack of memory would be good. There's little we can do in that situation, and in my experience this most likely point to a memory leak. So better follow a less agressive way than filling the logs, OK? We indeed have a way to report this via the existing -s options. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 April 2017 at 15:16, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Yes, but this is going to full the logs if ever happen. > > Better add stats: > > /* statistics */ > struct { > uint64_t msg_rcv_malformed; > uint32_t msg_rcv_bad_version; > uint32_t msg_rcv_bad_payload; > uint32_t msg_rcv_bad_header; > uint32_t msg_rcv_bad_type; > uint32_t msg_rcv_truncated; > uint32_t msg_rcv_bad_size; > uint32_t msg_snd_malformed; > uint64_t msg_rcv_lost; > uint64_t msg_rcv_before; > } error; > > A quick glance at the code to see how we're globaling deal with lack > of memory would be good. There's little we can do in that situation, > and in my experience this most likely point to a memory leak. > > So better follow a less agressive way than filling the logs, OK? We > indeed have a way to report this via the existing -s options. Ok, then I can drop this patch from the series and add later a couple more of stats. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 02, 2017 at 10:34:12AM +0200, Arturo Borrero Gonzalez wrote: > On 25 April 2017 at 15:16, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > Yes, but this is going to full the logs if ever happen. > > > > Better add stats: > > > > /* statistics */ > > struct { > > uint64_t msg_rcv_malformed; > > uint32_t msg_rcv_bad_version; > > uint32_t msg_rcv_bad_payload; > > uint32_t msg_rcv_bad_header; > > uint32_t msg_rcv_bad_type; > > uint32_t msg_rcv_truncated; > > uint32_t msg_rcv_bad_size; > > uint32_t msg_snd_malformed; > > uint64_t msg_rcv_lost; > > uint64_t msg_rcv_before; > > } error; > > > > A quick glance at the code to see how we're globaling deal with lack > > of memory would be good. There's little we can do in that situation, > > and in my experience this most likely point to a memory leak. > > > > So better follow a less agressive way than filling the logs, OK? We > > indeed have a way to report this via the existing -s options. > > Ok, then I can drop this patch from the series and add later a couple > more of stats. Great. Thanks Arturo. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 02, 2017 at 10:34:12AM +0200, Arturo Borrero Gonzalez wrote: > On 25 April 2017 at 15:16, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > Yes, but this is going to full the logs if ever happen. > > > > Better add stats: > > > > /* statistics */ > > struct { > > uint64_t msg_rcv_malformed; > > uint32_t msg_rcv_bad_version; > > uint32_t msg_rcv_bad_payload; > > uint32_t msg_rcv_bad_header; > > uint32_t msg_rcv_bad_type; > > uint32_t msg_rcv_truncated; > > uint32_t msg_rcv_bad_size; > > uint32_t msg_snd_malformed; > > uint64_t msg_rcv_lost; > > uint64_t msg_rcv_before; > > } error; > > > > A quick glance at the code to see how we're globaling deal with lack > > of memory would be good. There's little we can do in that situation, > > and in my experience this most likely point to a memory leak. > > > > So better follow a less agressive way than filling the logs, OK? We > > indeed have a way to report this via the existing -s options. > > Ok, then I can drop this patch from the series and add later a couple > more of stats. Please do. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/channel.c b/src/channel.c index acbfa7d..b2f114d 100644 --- a/src/channel.c +++ b/src/channel.c @@ -19,6 +19,7 @@ #include "channel.h" #include "network.h" #include "queue.h" +#include "log.h" static struct channel_ops *ops[CHANNEL_MAX]; extern struct channel_ops channel_mcast; @@ -161,8 +162,11 @@ static void channel_enqueue_errors(struct channel *c) struct channel_error *error; qobj = queue_object_new(Q_ELEM_ERR, sizeof(struct channel_error)); - if (qobj == NULL) + if (qobj == NULL) { + dlog(LOG_WARNING, "could not enqueue channel errors, failed to" + " allocate memory"); return; + } error = (struct channel_error *)qobj->data; error->len = c->buffer->len; diff --git a/src/queue_tx.c b/src/queue_tx.c index 0c99163..83eb111 100644 --- a/src/queue_tx.c +++ b/src/queue_tx.c @@ -22,6 +22,7 @@ #include "queue.h" #include "conntrackd.h" #include "network.h" +#include "log.h" void tx_queue_add_ctlmsg(uint32_t flags, uint32_t from, uint32_t to) { @@ -29,8 +30,11 @@ void tx_queue_add_ctlmsg(uint32_t flags, uint32_t from, uint32_t to) struct nethdr_ack *ack; qobj = queue_object_new(Q_ELEM_CTL, sizeof(struct nethdr_ack)); - if (qobj == NULL) + if (qobj == NULL) { + dlog(LOG_WARNING, "could not queue ACK message. Failed to " + "allocate memory"); return; + } ack = (struct nethdr_ack *)qobj->data; ack->type = NET_T_CTL; @@ -48,8 +52,11 @@ void tx_queue_add_ctlmsg2(uint32_t flags) struct nethdr *ctl; qobj = queue_object_new(Q_ELEM_CTL, sizeof(struct nethdr_ack)); - if (qobj == NULL) + if (qobj == NULL) { + dlog(LOG_WARNING, "could not queue CTL message. Failed to " + "allocate memory"); return; + } ctl = (struct nethdr *)qobj->data; ctl->type = NET_T_CTL;
These warnings, if they happen, should help users. Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org> --- src/channel.c | 6 +++++- src/queue_tx.c | 11 +++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html