> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Wednesday, 17 May 2023 18.16
>
> The term sanity check is on the Tier 2 list of words
> that should be replaced.
>
> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> ---
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
> -----
>
> -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))
>
> #else /* RTE_LIBRTE_AVP_DEBUG_BUFFERS */
>
> -#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;
> }
>
> -/* 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)
>
> #ifdef RTE_LIBRTE_MBUF_DEBUG
>
> -/** 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)
>
> #else /* RTE_LIBRTE_MBUF_DEBUG */
>
> -/** 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)
>
> #endif /* RTE_LIBRTE_MBUF_DEBUG */
>
> @@ -514,7 +514,7 @@ rte_mbuf_ext_refcnt_update(struct
> rte_mbuf_ext_shared_info *shinfo,
>
>
> /**
> - * 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);
> +}
>
> /**
> - * 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);
>
> /**
> - * 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.
>
> local: *;
> };
