Hi Dmitry,
Thanks for your feedback!
On Fri, Aug 13, 2021 at 11:59 AM Dmitry Kozlyuk
<dmitry.kozl...@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.
