[2/3] mbuf: add a non fatal sanity check helper
diff mbox series

Message ID 20180910054547.18494-3-david.marchand@6wind.com
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • segment sanity checks
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

David Marchand Sept. 10, 2018, 5:45 a.m. UTC
Let's add a little helper that does the same as rte_mbuf_sanity_check but
without the panic.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_mbuf/Makefile             |  2 +
 lib/librte_mbuf/meson.build          |  2 +
 lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++--------
 lib/librte_mbuf/rte_mbuf.h           | 23 +++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++
 5 files changed, 86 insertions(+), 21 deletions(-)

Comments

David Marchand Sept. 10, 2018, 5:56 a.m. UTC | #1
On Mon, Sep 10, 2018 at 7:45 AM, David Marchand
<david.marchand@6wind.com> wrote:
> Let's add a little helper that does the same as rte_mbuf_sanity_check but
> without the panic.
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_mbuf/Makefile             |  2 +
>  lib/librte_mbuf/meson.build          |  2 +
>  lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++--------
>  lib/librte_mbuf/rte_mbuf.h           | 23 +++++++++
>  lib/librte_mbuf/rte_mbuf_version.map |  6 +++

Applied a little patch on devtools/check-symbol-change.sh
@@ -115,7 +115,7 @@ check_for_rule_violations()
                                if [ $? -ne 0 ]
                                then
                                        echo -n "ERROR: symbol $symname "
-                                       echo -n "is added in a section "
+                                       echo -n "is added in $secname section "
                                        echo -n "other than the EXPERIMENTAL "
                                        echo "section of the version map"
                                        ret=1

And here is what I get when I check my patch with it:

marchand@gribouille:~/git/dpdk$ ./devtools/check-symbol-change.sh
sanity/0002-mbuf-add-a-non-fatal-sanity-check-helper.patch
ERROR: symbol rte_mbuf_check is added in +EXPERIMENTAL section other
than the EXPERIMENTAL section of the version map

Is this because the EXPERIMENTAL section is added in the same patch ?
Andrew Rybchenko Sept. 10, 2018, 8:12 a.m. UTC | #2
On 09/10/2018 08:45 AM, David Marchand wrote:
> Let's add a little helper that does the same as rte_mbuf_sanity_check but
> without the panic.
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---

<...>

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index a50b05c64..e12a4c765 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -977,6 +977,29 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
>   void
>   rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header);
>   
> +/**
> + * Sanity checks on a mbuf.
> + *
> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> + * if corruption is detected rather than panic.
> + *
> + * @param m
> + *   The mbuf to be checked.
> + * @param is_header
> + *   True if the mbuf is a packet header, false if it is a sub-segment
> + *   of a packet (in this case, some fields like nb_segs are not checked)
> + * @param reason
> + *   A reference to a string pointer where to store the reason why a mbuf is
> + *   considered invalid.
> + * @return
> + *   - 0 if no issue has been found, reason is left untouched.
> + *   - -1 if a problem is detected, reason then points to a string describing
> + *     the reason why the mbuf is deemed invalid.
> + */
> +__rte_experimental
> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> +		   const char **reason);
> +

May be it would be better to return reason as return value and if it is 
NULL everything is OK?

<...>
David Marchand Sept. 10, 2018, 8:24 a.m. UTC | #3
On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
> +/**
> + * Sanity checks on a mbuf.
> + *
> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> + * if corruption is detected rather than panic.
> + *
> + * @param m
> + *   The mbuf to be checked.
> + * @param is_header
> + *   True if the mbuf is a packet header, false if it is a sub-segment
> + *   of a packet (in this case, some fields like nb_segs are not checked)
> + * @param reason
> + *   A reference to a string pointer where to store the reason why a mbuf
> is
> + *   considered invalid.
> + * @return
> + *   - 0 if no issue has been found, reason is left untouched.
> + *   - -1 if a problem is detected, reason then points to a string
> describing
> + *     the reason why the mbuf is deemed invalid.
> + */
> +__rte_experimental
> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> +   const char **reason);
> +
>
>
> May be it would be better to return reason as return value and if it is NULL
> everything is OK?

This was what I had done with my first attempt.
But given the name "rte_mbuf_check", having a int (used as a bool)
returned makes sense to me.

So no strong opinion here.
Andrew Rybchenko Sept. 10, 2018, 8:33 a.m. UTC | #4
On 09/10/2018 11:24 AM, David Marchand wrote:
> On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
>> +/**
>> + * Sanity checks on a mbuf.
>> + *
>> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
>> + * if corruption is detected rather than panic.
>> + *
>> + * @param m
>> + *   The mbuf to be checked.
>> + * @param is_header
>> + *   True if the mbuf is a packet header, false if it is a sub-segment
>> + *   of a packet (in this case, some fields like nb_segs are not checked)
>> + * @param reason
>> + *   A reference to a string pointer where to store the reason why a mbuf
>> is
>> + *   considered invalid.
>> + * @return
>> + *   - 0 if no issue has been found, reason is left untouched.
>> + *   - -1 if a problem is detected, reason then points to a string
>> describing
>> + *     the reason why the mbuf is deemed invalid.
>> + */
>> +__rte_experimental
>> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>> +   const char **reason);
>> +
>>
>>
>> May be it would be better to return reason as return value and if it is NULL
>> everything is OK?
> This was what I had done with my first attempt.
> But given the name "rte_mbuf_check", having a int (used as a bool)
> returned makes sense to me.

Yes, good point.

> So no strong opinion here.

Me too.
Olivier Matz Oct. 9, 2018, 9:10 a.m. UTC | #5
On Mon, Sep 10, 2018 at 11:33:44AM +0300, Andrew Rybchenko wrote:
> On 09/10/2018 11:24 AM, David Marchand wrote:
> > On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> > > +/**
> > > + * Sanity checks on a mbuf.
> > > + *
> > > + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> > > + * if corruption is detected rather than panic.
> > > + *
> > > + * @param m
> > > + *   The mbuf to be checked.
> > > + * @param is_header
> > > + *   True if the mbuf is a packet header, false if it is a sub-segment
> > > + *   of a packet (in this case, some fields like nb_segs are not checked)
> > > + * @param reason
> > > + *   A reference to a string pointer where to store the reason why a mbuf
> > > is
> > > + *   considered invalid.
> > > + * @return
> > > + *   - 0 if no issue has been found, reason is left untouched.
> > > + *   - -1 if a problem is detected, reason then points to a string
> > > describing
> > > + *     the reason why the mbuf is deemed invalid.
> > > + */
> > > +__rte_experimental
> > > +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> > > +   const char **reason);
> > > +
> > > 
> > > 
> > > May be it would be better to return reason as return value and if it is NULL
> > > everything is OK?
> > This was what I had done with my first attempt.
> > But given the name "rte_mbuf_check", having a int (used as a bool)
> > returned makes sense to me.
> 
> Yes, good point.
> 
> > So no strong opinion here.
> 
> Me too.

I think an int is fine.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Patch
diff mbox series

diff --git a/lib/librte_mbuf/Makefile b/lib/librte_mbuf/Makefile
index e2b98a254..7d0c824a3 100644
--- a/lib/librte_mbuf/Makefile
+++ b/lib/librte_mbuf/Makefile
@@ -7,6 +7,8 @@  include $(RTE_SDK)/mk/rte.vars.mk
 LIB = librte_mbuf.a
 
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
+CFLAGS += -DALLOW_EXPERIMENTAL_API
+
 LDLIBS += -lrte_eal -lrte_mempool
 
 EXPORT_MAP := rte_mbuf_version.map
diff --git a/lib/librte_mbuf/meson.build b/lib/librte_mbuf/meson.build
index 45ffb0db5..4d9bdf2ba 100644
--- a/lib/librte_mbuf/meson.build
+++ b/lib/librte_mbuf/meson.build
@@ -5,3 +5,5 @@  version = 3
 sources = files('rte_mbuf.c', 'rte_mbuf_ptype.c', 'rte_mbuf_pool_ops.c')
 headers = files('rte_mbuf.h', 'rte_mbuf_ptype.h', 'rte_mbuf_pool_ops.h')
 deps += ['mempool']
+
+allow_experimental_apis = true
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 137a320ed..4e65f0f82 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -170,49 +170,81 @@  rte_pktmbuf_pool_create(const char *name, unsigned int n,
 /* do some sanity checks on a mbuf: panic if it fails */
 void
 rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
+{
+	const char *reason;
+
+	if (rte_mbuf_check(m, is_header, &reason))
+		rte_panic("%s\n", reason);
+}
+
+__rte_experimental
+int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
+		   const char **reason)
 {
 	unsigned int nb_segs, pkt_len;
 
-	if (m == NULL)
-		rte_panic("mbuf is NULL\n");
+	if (m == NULL) {
+		*reason = "mbuf is NULL";
+		return -1;
+	}
 
 	/* generic checks */
-	if (m->pool == NULL)
-		rte_panic("bad mbuf pool\n");
-	if (m->buf_iova == 0)
-		rte_panic("bad IO addr\n");
-	if (m->buf_addr == NULL)
-		rte_panic("bad virt addr\n");
+	if (m->pool == NULL) {
+		*reason = "bad mbuf pool";
+		return -1;
+	}
+	if (m->buf_iova == 0) {
+		*reason = "bad IO addr";
+		return -1;
+	}
+	if (m->buf_addr == NULL) {
+		*reason = "bad virt addr";
+		return -1;
+	}
 
 	uint16_t cnt = rte_mbuf_refcnt_read(m);
-	if ((cnt == 0) || (cnt == UINT16_MAX))
-		rte_panic("bad ref cnt\n");
+	if ((cnt == 0) || (cnt == UINT16_MAX)) {
+		*reason = "bad ref cnt";
+		return -1;
+	}
 
 	/* nothing to check for sub-segments */
 	if (is_header == 0)
-		return;
+		return 0;
 
 	/* data_len is supposed to be not more than pkt_len */
-	if (m->data_len > m->pkt_len)
-		rte_panic("bad data_len\n");
+	if (m->data_len > m->pkt_len) {
+		*reason = "bad data_len";
+		return -1;
+	}
 
 	nb_segs = m->nb_segs;
 	pkt_len = m->pkt_len;
 
 	do {
-		if (m->data_off > m->buf_len)
-			rte_panic("data offset too big in mbuf segment\n");
+		if (m->data_off > m->buf_len) {
+			*reason = "data offset too big in mbuf segment";
+			return -1;
+		}
 		if ((uint32_t)m->data_off + (uint32_t)m->data_len >
-				(uint32_t)m->buf_len)
-			rte_panic("data length too big in mbuf segment\n");
+				(uint32_t)m->buf_len) {
+			*reason = "data length too big in mbuf segment";
+			return -1;
+		}
 		nb_segs -= 1;
 		pkt_len -= m->data_len;
 	} while ((m = m->next) != NULL);
 
-	if (nb_segs)
-		rte_panic("bad nb_segs\n");
-	if (pkt_len)
-		rte_panic("bad pkt_len\n");
+	if (nb_segs) {
+		*reason = "bad nb_segs";
+		return -1;
+	}
+	if (pkt_len) {
+		*reason = "bad pkt_len";
+		return -1;
+	}
+
+	return 0;
 }
 
 /* dump a mbuf on console */
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a50b05c64..e12a4c765 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -977,6 +977,29 @@  rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 void
 rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header);
 
+/**
+ * Sanity checks on a mbuf.
+ *
+ * Almost like rte_mbuf_sanity_check(), but this function gives the reason
+ * if corruption is detected rather than panic.
+ *
+ * @param m
+ *   The mbuf to be checked.
+ * @param is_header
+ *   True if the mbuf is a packet header, false if it is a sub-segment
+ *   of a packet (in this case, some fields like nb_segs are not checked)
+ * @param reason
+ *   A reference to a string pointer where to store the reason why a mbuf is
+ *   considered invalid.
+ * @return
+ *   - 0 if no issue has been found, reason is left untouched.
+ *   - -1 if a problem is detected, reason then points to a string describing
+ *     the reason why the mbuf is deemed invalid.
+ */
+__rte_experimental
+int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
+		   const char **reason);
+
 #define MBUF_RAW_ALLOC_CHECK(m) do {				\
 	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
 	RTE_ASSERT((m)->next == NULL);				\
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index cae68db8d..2662a37bf 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -45,3 +45,9 @@  DPDK_18.08 {
 	rte_mbuf_user_mempool_ops;
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
+
+EXPERIMENTAL {
+	global:
+
+	rte_mbuf_check;
+} DPDK_18.08;