From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D79DBA0C4B; Sat, 14 Aug 2021 04:32:21 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 203B24068F; Sat, 14 Aug 2021 04:32:21 +0200 (CEST) Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by mails.dpdk.org (Postfix) with ESMTP id 18A0D4013F for ; Sat, 14 Aug 2021 04:32:19 +0200 (CEST) Received: by mail-pj1-f54.google.com with SMTP id nt11so18057811pjb.2 for ; Fri, 13 Aug 2021 19:32:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Lda9OKiDGfO1cXLbH1RVBKPLLLvpnZVjmF2jLLty0mU=; b=DcnWNFJDDAN2lI/Pq9Q+6yjpz4Hsny8i/I0tEpJFyp+n0Yjo9MD0teUx6837gTlVNN p5eLfSoucAqxPeI4GfN4+8rbjD/WN0v29x73rr3FPaUtFQ+dh59L1PhVeX26E/ulcQq1 eG6QD7qLjw+9mAYG3TJVwVQB+0HB77H38hMW2vE1dDAbop048hd84cYU6gj58UqKXouQ RzdSyao4Zz+Q5O1b+qA9R+ikxuFn4u0fGapXUhMoIgJ3f1T4u9yHyV8mpv6i288P8VW2 aefOGt3IkCAAR1G/NG9zPpJMZd3bVHzt5YvfBKDFubrJzxPBkbtWn5aS5YfPYDz23JRP dJng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Lda9OKiDGfO1cXLbH1RVBKPLLLvpnZVjmF2jLLty0mU=; b=PsO1T0ILkX9MKQ1J13j6TVvPxIft+hGtqE8ciYe6+rrgbGxFbwFDEQ5r4tBiI+/UJF NIT+PKYqJOzWEVQBu4ahQVRfmrXMQQSigyuY/HGmZzVtKlTTVrMoTuVzQSAjgUmTp2rN JpVdyrYF74hCX8Muf7bvH24fYardgqqDDwHlbDR6q+gwzLuN7+UGKQhEQHl6RI1CaqgV ZNUbfAal74W6a1UsmvyveVCuX1iDYRo9xXp45hQwyStyGtKOxMkjQn4kZDyoRjH4sMqr OKsa5pLzOYnqT2FJWhb5zTHYgeJ/ysMVGcr7xZsrQTCIN4DM8mbRh2xBpkInir1n9405 xdPQ== X-Gm-Message-State: AOAM531sbh3B/qoN2UoaLxCR+FqqmKL3NdxHvNHIdAUeSfYOMXL2WoEK Y0LFMGBWnuA07Sa4yFzh/0TfXz8XkbY/uN98ac8= X-Google-Smtp-Source: ABdhPJyFbQVSgM5QW3RRyNSUfrAaBIaYdqOWeAnDMMq77lwNdE/mWisv/EyY2QH25pZ+Fy7wzg266iQ5Bz4zVWQroVA= X-Received: by 2002:a17:90a:1a51:: with SMTP id 17mr5296185pjl.59.1628908338215; Fri, 13 Aug 2021 19:32:18 -0700 (PDT) MIME-Version: 1.0 References: <20210813010250.87528-1-u9012063@gmail.com> <20210813033627.78546-1-u9012063@gmail.com> <20210813215907.797e4553@sovereign> In-Reply-To: <20210813215907.797e4553@sovereign> From: William Tu Date: Fri, 13 Aug 2021 19:31:41 -0700 Message-ID: To: Dmitry Kozlyuk Cc: dpdk-dev , Nick Connolly , Stephen Hemminger Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCHv5] eal: remove sys/queue.h from public headers. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Dmitry, Thanks for your feedback! On Fri, Aug 13, 2021 at 11:59 AM Dmitry Kozlyuk 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 " 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 > > +#include > > + > > +/* 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.