> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozl...@gmail.com>
> Sent: Wednesday, October 20, 2021 2:58 AM
> To: Harman Kalra <hka...@marvell.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richard...@intel.com>;
> david.march...@redhat.com; m...@ashroe.eu; tho...@monjalon.net
> Subject: [EXT] Re: [PATCH v4 3/7] eal/interrupts: avoid direct access to
> interrupt handle
>
> External Email
>
> ----------------------------------------------------------------------
> 2021-10-20 00:05 (UTC+0530), Harman Kalra:
> > Making changes to the interrupt framework to use interrupt handle APIs
> > to get/set any field. Direct access to any of the fields should be
> > avoided to avoid any ABI breakage in future.
>
> I get and accept the point why EAL also should use the API.
> However, mentioning ABI is still a wrong wording.
> There is no ABI between EAL structures and EAL functions by definition of
> ABI.
Sure, I will reword the commit message without ABI inclusion.
>
> >
> > Signed-off-by: Harman Kalra <hka...@marvell.com>
> > ---
> > lib/eal/freebsd/eal_interrupts.c | 92 ++++++----
> > lib/eal/linux/eal_interrupts.c | 287 +++++++++++++++++++------------
> > 2 files changed, 234 insertions(+), 145 deletions(-)
> >
> > diff --git a/lib/eal/freebsd/eal_interrupts.c
> > b/lib/eal/freebsd/eal_interrupts.c
> [...]
> > @@ -135,9 +137,18 @@ rte_intr_callback_register(const struct
> rte_intr_handle *intr_handle,
> > ret = -ENOMEM;
> > goto fail;
> > } else {
> > - src->intr_handle = *intr_handle;
> > - TAILQ_INIT(&src->callbacks);
> > - TAILQ_INSERT_TAIL(&intr_sources, src, next);
> > + src->intr_handle = rte_intr_instance_alloc();
> > + if (src->intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Can not create
> intr instance\n");
> > + free(callback);
> > + ret = -ENOMEM;
>
> goto fail?
I think goto not required, as we not setting wake_thread = 1 here,
API will just return error after unlocking the spinlock and trace.
>
> > + } else {
> > + rte_intr_instance_copy(src-
> >intr_handle,
> > + intr_handle);
> > + TAILQ_INIT(&src->callbacks);
> > + TAILQ_INSERT_TAIL(&intr_sources,
> src,
> > + next);
> > + }
> > }
> > }
> >
> [...]
> > @@ -213,7 +226,7 @@ rte_intr_callback_unregister_pending(const struct
> rte_intr_handle *intr_handle,
> > struct rte_intr_callback *cb, *next;
> >
> > /* do parameter checking first */
> > - if (intr_handle == NULL || intr_handle->fd < 0) {
> > + if (intr_handle == NULL || rte_intr_fd_get(intr_handle) < 0) {
>
> The handle is checked for NULL inside the accessor, here and in other places:
> grep -R 'intr_handle == NULL ||' lib/eal
Ack, I will remove these NULL checks.
>
> > RTE_LOG(ERR, EAL,
> > "Unregistering with invalid input parameter\n");
> > return -EINVAL;
>
> > diff --git a/lib/eal/linux/eal_interrupts.c
> > b/lib/eal/linux/eal_interrupts.c
> [...]
