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

[William Tu <u9012063@gmail.com>]

Hi Dmitry,
Thanks for your feedback.

On Thu, Aug 19, 2021 at 4:29 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2021-08-18 23:26 (UTC+0000), William Tu:
> [...]
> > diff --git a/lib/eal/linux/include/rte_os.h b/lib/eal/linux/include/rte_os.h
> > index 1618b4df22..ce5b0aed52 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) \
> > +     for ((var) = TAILQ_FIRST((head)); \
> > +         (var) && ((tvar) = TAILQ_NEXT((var), field), 1); \
> > +         (var) = (tvar))
>
> I made a comment to v5 and you were going to fix it, maybe it got lost?
>
>         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
>         [in rte_tailq.h].
>
> The important part is duplication in rte_os.h for each platform
> and the loss of documentation. I see you already removed #ifdef.

Yes, it got lost. I will fix the documentation, and the
RTE_TAILQ_FOREACH_SAFE at rte_tailq.h.

>
> > +#define RTE_TAILQ_FIRST(head) TAILQ_FIRST(head)
> > +#define RTE_TAILQ_NEXT(elem, field) TAILQ_NEXT(elem, field)
> > +#define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
> > +#define RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
> > +
> >
> >  #ifdef CPU_SETSIZE /* may require _GNU_SOURCE */
> >  typedef cpu_set_t rte_cpuset_t;
> > diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> > index e5dc54efb8..103c1f909d 100644
> > --- a/lib/eal/windows/eal_alarm.c
> > +++ b/lib/eal/windows/eal_alarm.c
> > @@ -4,6 +4,7 @@
> >
> >  #include <stdatomic.h>
> >  #include <stdbool.h>
> > +#include <sys/queue.h>
> >
> >  #include <rte_alarm.h>
> >  #include <rte_spinlock.h>
> > diff --git a/lib/eal/windows/include/rte_os.h b/lib/eal/windows/include/rte_os.h
> > index 66c711d458..0cbe1dbc1e 100644
> > --- a/lib/eal/windows/include/rte_os.h
> > +++ b/lib/eal/windows/include/rte_os.h
> > @@ -18,6 +18,37 @@
> >  extern "C" {
> >  #endif
> >
> > +/* These macros are compatible with bundled sys/queue.h. */
> > +#define RTE_TAILQ_HEAD(name, type) \
> > +struct name { \
> > +     struct type *tqh_first; /* first element */ \
> > +     struct type **tqh_last; /* addr of last next element */ \
> > +}
> > +#define RTE_TAILQ_ENTRY(type) \
> > +struct { \
> > +     struct type *tqe_next; /* next element */ \
> > +     struct type **tqe_prev; /* address of previous next element */ \
> > +}
> > +#define RTE_TAILQ_FOREACH(var, head, field) \
> > +     for ((var) = RTE_TAILQ_FIRST((head)); \
> > +         (var); \
> > +         (var) = RTE_TAILQ_NEXT((var), field))
> > +#define RTE_TAILQ_FOREACH_SAFE(var, head, field, tvar) \
> > +     for ((var) = TAILQ_FIRST((head)); \
> > +         (var) && ((tvar) = TAILQ_NEXT((var), field), 1); \
> > +         (var) = (tvar))
> > +#define RTE_TAILQ_FIRST(head) ((head)->tqh_first)
> > +#define RTE_TAILQ_NEXT(elm, field) ((elm)->field.tqe_next)
> > +#define RTE_STAILQ_HEAD(name, type) \
> > +struct name { \
> > +     struct type *stqh_first;/* first element */ \
> > +     struct type **stqh_last;/* addr of last next element */ \
> > +}
> > +#define RTE_STAILQ_ENTRY(type) \
> > +struct { \
> > +     struct type *stqe_next; /* next element */ \
> > +}
> > +
>
> Please drop the inline comments.
> They duplicate what's already in sys/queue.h
> and we're not going to maintain them.

OK, I will remove all the inline comments.

Thanks,
William

William