From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3136542B2F; Thu, 18 May 2023 00:58:41 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9AA840E25; Thu, 18 May 2023 00:58:40 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 66FCF40698 for ; Thu, 18 May 2023 00:58:39 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id C45B622537; Thu, 18 May 2023 00:58:36 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH 01/20] mbuf: replace term sanity check X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Thu, 18 May 2023 00:58:34 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8792F@smartserver.smartshare.dk> In-Reply-To: <20230517161603.117728-2-stephen@networkplumber.org> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH 01/20] mbuf: replace term sanity check Thread-Index: AdmI2uqqAQx6wuLfRXSjajcoAVgUYQAMmAlQ References: <20230517161603.117728-1-stephen@networkplumber.org> <20230517161603.117728-2-stephen@networkplumber.org> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Stephen Hemminger" , Cc: "Olivier Matz" , "Steven Webster" , "Matt Peters" , "Andrew Rybchenko" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, 17 May 2023 18.16 >=20 > The term sanity check is on the Tier 2 list of words > that should be replaced. >=20 > Signed-off-by: Stephen Hemminger > --- Naming is hard... I have shared a bunch of unfiltered thoughts below, = mainly about trying to reuse the same terms everywhere. The quality = varies, so just disregard the comments you consider silly. Anyway, thank you for the effort put into this, Stephen! [...] > - rte_mbuf_sanity_check(m, 1); > - rte_mbuf_sanity_check(m, 0); > + rte_mbuf_validate(m, 1); > + rte_mbuf_validate(m, 0); Note: "mbuf_sanity_check()" -> "mbuf_validate()" [...] > --- a/doc/guides/prog_guide/mbuf_lib.rst > +++ b/doc/guides/prog_guide/mbuf_lib.rst > @@ -266,8 +266,8 @@ can be found in several of the sample = applications, > for example, the IPv4 Multic > Debug > ----- >=20 > -In debug mode, the functions of the mbuf library perform sanity = checks > before any operation (such as, buffer corruption, > -bad type, and so on). > +In debug mode, the functions of the mbuf library perform consistency > checks > +before any operation (such as, buffer corruption, bad type, and so = on). Note: "sanity checks" -> "consistency checks" [...] > -#define avp_dev_buffer_sanity_check(a, b) \ > - __avp_dev_buffer_sanity_check((a), (b)) > +#define avp_dev_buffer_check(a, b) \ > + __avp_dev_buffer_check((a), (b)) >=20 > #else /* RTE_LIBRTE_AVP_DEBUG_BUFFERS */ >=20 > -#define avp_dev_buffer_sanity_check(a, b) do {} while (0) > +#define avp_dev_buffer_check(a, b) do {} while (0) Note: "buffer_sanity_check()" -> "buffer_check()" Shouldn't these become "buffer_validate()" instead of "buffer_check()"? [...] > +++ b/lib/mbuf/rte_mbuf.c > @@ -363,9 +363,9 @@ rte_pktmbuf_pool_create_extbuf(const char *name, > unsigned int n, > return mp; > } >=20 > -/* do some sanity checks on a mbuf: panic if it fails */ > +/* do some checks on a mbuf: panic if it fails */ Trying to make the description resemble the function name, suggest: = "validate mbuf consistency; panic if it fails" > void > -rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header) > +rte_mbuf_validate(const struct rte_mbuf *m, int is_header) [...] > +++ b/lib/mbuf/rte_mbuf.h > @@ -339,13 +339,13 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp) >=20 > #ifdef RTE_LIBRTE_MBUF_DEBUG >=20 > -/** check mbuf type in debug mode */ > -#define __rte_mbuf_sanity_check(m, is_h) rte_mbuf_sanity_check(m, = is_h) > +/** do mbuf type in debug mode */ "do mbuf type" -> "validate mbuf consistency" > +#define __rte_mbuf_validate(m, is_h) rte_mbuf_validate(m, is_h) >=20 > #else /* RTE_LIBRTE_MBUF_DEBUG */ >=20 > -/** check mbuf type in debug mode */ > -#define __rte_mbuf_sanity_check(m, is_h) do { } while (0) > +/** ignore mbuf checks if not in debug mode */ Good improvement mentioning that they are ignored. Trying to make the description resemble the function name, suggest: = "ignore mbuf consistency validation if not in debug mode" > +#define __rte_mbuf_validate(m, is_h) do { } while (0) >=20 > #endif /* RTE_LIBRTE_MBUF_DEBUG */ >=20 > @@ -514,7 +514,7 @@ rte_mbuf_ext_refcnt_update(struct > rte_mbuf_ext_shared_info *shinfo, >=20 >=20 > /** > - * Sanity checks on an mbuf. > + * Consistency checks on an mbuf. Trying to make the description resemble the function name, suggest: = "Validate mbuf consistency." > * > * Check the consistency of the given mbuf. The function will cause a > * panic if corruption is detected. > @@ -526,12 +526,19 @@ rte_mbuf_ext_refcnt_update(struct > rte_mbuf_ext_shared_info *shinfo, > * of a packet (in this case, some fields like nb_segs are not > checked) > */ > void > -rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header); > +rte_mbuf_validate(const struct rte_mbuf *m, int is_header); > + > +/* Older deprecated name for rte_mbuf_validate() */ > +static inline __rte_deprecated > +void rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header) > +{ > + rte_mbuf_validate(m, is_header); > +} >=20 > /** > - * Sanity checks on a mbuf. > + * Do consistency checks on a mbuf. Trying to make the description resemble the function name, suggest: = "Validate mbuf consistency." Nothing wrong with using the exact same = description headline. > * > - * Almost like rte_mbuf_sanity_check(), but this function gives the > reason > + * Almost like rte_mbuf_validate(), but this function gives the = reason > * if corruption is detected rather than panic. > * > * @param m > @@ -551,7 +558,7 @@ int rte_mbuf_check(const struct rte_mbuf *m, int > is_header, > const char **reason); >=20 > /** > - * Sanity checks on a reinitialized mbuf in debug mode. > + * Do checks on a reinitialized mbuf in debug mode. Trying to make the description resemble the function name, suggest: = "Validate consistency of a reinitialized mbuf in debug mode." > * > * Check the consistency of the given reinitialized mbuf. > * The function will cause a panic if corruption is detected. [...] > /** For backwards compatibility. */ > -#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m) > +#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_validate(m) This macro is not used in DPDK, and we are renaming the related = functions anyway; let's remove this macro while we are at it. (The macro = also lacks the RTE_ prefix.) [...] > +++ b/lib/mbuf/version.map > @@ -27,7 +27,6 @@ DPDK_23 { > rte_mbuf_dynflag_register; > rte_mbuf_dynflag_register_bitnum; > rte_mbuf_platform_mempool_ops; > - rte_mbuf_sanity_check; > rte_mbuf_set_platform_mempool_ops; > rte_mbuf_set_user_mempool_ops; > rte_mbuf_user_mempool_ops; > @@ -39,6 +38,7 @@ DPDK_23 { > rte_pktmbuf_pool_create; > rte_pktmbuf_pool_create_by_ops; > rte_pktmbuf_pool_init; > + rte_mbuf_validate; The public function is renamed, so it belongs in the next API version, = not DPDK_23. >=20 > local: *; > };