Re: [dpdk-dev] [PATCHv5] eal: remove sys/queue.h from public headers.

[William Tu <u9012063@gmail.com>]

Hi Dmitry,
Thanks for your feedback!

On Fri, Aug 13, 2021 at 11:59 AM Dmitry Kozlyuk
<dmitry.kozliuk@gmail.com> wrote:
>
> 2021-08-13 03:36 (UTC+0000), William Tu:
> > Currently there are some public headers that include 'sys/queue.h', which
> > is not POSIX, but usually provided by the Linux/BSD system library.
> > (Not in POSIX.1, POSIX.1-2001, or POSIX.1-2008. Present on the BSDs.)
> > The file is missing on Windows. During the windows build, DPDK uses a
>
> Typo: "Windows".
>
> > bundled copy, so building a DPDK library works fine.  But when OVS or other
> > applications use DPDK as a library, because some DPDK public headers
> > include 'sys/queue.h', on Windows, it triggers an error due to no such file.
> >
> > One solution is to install the 'lib/eal/windows/include/sys/queue.h' into
> > Windows environment, such as [1]. However, this means DPDK exports the
> > functionalities of 'sys/queue.h' into the environment, which might cause
> > symbols, macros, headers clashing with other applications.
> >
> > The patch fixes it by removing the "#include <sys/queue.h>" from
> > DPDK public headers, so programs including DPDK headers don't depend
> > on the system to provide 'sys/queue.h'. When these public headers use
> > macros such as TAILQ_xxx, we replace it with RTE_ prefix.
>
> "replace it by _the ones_ with RTE_ prefix"?
OK
[...]
> 1. Please register at http://patchwork.dpdk.org with the email used for the
> patches and update the state of all previous versions to "Superseded".
> It is not currently done automatically and only you and a few maintainers
> can change the state.
Done

>
> Patchwork also shows CI build failures with v5, they need to be fixed.
>
> 2. Are you using `git format-patch -v5 ...` to create patches?
OK, will use it.

> The subject of your patches is missing a space ("PATCH v5" vs "PATCHv5").
> Not sure if tools like patchwork will properly process it.
>
> [...]
> >  struct rte_afu_driver {
> > -     TAILQ_ENTRY(rte_afu_driver) next;       /**< Next afu driver. */
> > +     RTE_TAILQ_ENTRY(rte_afu_driver) next;       /**< Next afu driver. */
> >       struct rte_driver driver;               /**< Inherit core driver. */
> >       afu_probe_t *probe;                     /**< Device Probe function. */
> >       afu_remove_t *remove;                   /**< Device Remove function. */
>
> Re: loss of comment alignment here and in other places.
> Firstly, it's definitely not a big deal. Current patch is good because it only
> changes relevant lines. Re-aligning all the comments would be worse IMO.
> However, in cases like this, when keeping alignment doesn't require changing
> neighboring lines, it could be kept. Just a nit.
>
> [...]
> >  /* This macro permits both remove and free var within the loop safely.*/
> > -#ifndef TAILQ_FOREACH_SAFE
> > -#define TAILQ_FOREACH_SAFE(var, head, field, tvar)           \
> > -     for ((var) = TAILQ_FIRST((head));                       \
> > -         (var) && ((tvar) = TAILQ_NEXT((var), field), 1);    \
> > +#ifndef RTE_TAILQ_FOREACH_SAFE
> > +#define RTE_TAILQ_FOREACH_SAFE(var, head, field, tvar)               \
> > +     for ((var) = RTE_TAILQ_FIRST((head));                   \
> > +         (var) && ((tvar) = RTE_TAILQ_NEXT((var), field), 1);        \
> >           (var) = (tvar))
> >  #endif
>
> Why duplicate this in rte_os.h (documentation lost, BTW) and add #ifdef?
> RTE_TAILQ_FOREACH_SAFE is not needed in headers, it can be left here.
OK, will fix it.

>
> >
> > diff --git a/lib/eal/linux/include/rte_os.h b/lib/eal/linux/include/rte_os.h
> > index 1618b4df22..1a6e5b789f 100644
> > --- a/lib/eal/linux/include/rte_os.h
> > +++ b/lib/eal/linux/include/rte_os.h
> > @@ -11,6 +11,21 @@
> >   */
> >
> >  #include <sched.h>
> > +#include <sys/queue.h>
> > +
> > +/* These macros are compatible with system's sys/queue.h. */
> > +#define RTE_TAILQ_HEAD(name, type) TAILQ_HEAD(name, type)
> > +#define RTE_TAILQ_ENTRY(type) TAILQ_ENTRY(type)
> > +#define RTE_TAILQ_FOREACH(var, head, field) TAILQ_FOREACH(var, head, field)
> > +#define      RTE_TAILQ_FOREACH_SAFE(var, head, field, tvar) \
>
> Stray TAB here and in rte_os.h for other platforms.

Thanks, will fix it.