Message ID | 923ebe527219fa16ca91e74b416b978803056d70.1447829123.git.pmatilai@redhat.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 516C55A71; Wed, 18 Nov 2015 07:45:33 +0100 (CET) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id ABADD5A6C for <dev@dpdk.org>; Wed, 18 Nov 2015 07:45:32 +0100 (CET) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id ED0A98EFEB for <dev@dpdk.org>; Wed, 18 Nov 2015 06:45:31 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org.com (vpn1-7-206.ams2.redhat.com [10.36.7.206]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAI6jUce001626 for <dev@dpdk.org>; Wed, 18 Nov 2015 01:45:31 -0500 From: Panu Matilainen <pmatilai@redhat.com> To: dev@dpdk.org Date: Wed, 18 Nov 2015 08:45:23 +0200 Message-Id: <923ebe527219fa16ca91e74b416b978803056d70.1447829123.git.pmatilai@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 Subject: [dpdk-dev] [PATCH] eal: fix plugindir processing to be filesystem agnostic X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Panu Matilainen
Nov. 18, 2015, 6:45 a.m. UTC
Not all filesystems supply struct dirent d_type field, in which case
everything in the specified directory would go ignored. One such
filesystem being XFS which RHEL 7 defaults to... stat() the entries
instead.
Fixes: 9f8eb1d9ca0f ("eal: support driver loading from directory")
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
lib/librte_eal/common/eal_common_options.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Comments
Hello Panu, On Wed, Nov 18, 2015 at 7:45 AM, Panu Matilainen <pmatilai@redhat.com> wrote: > Not all filesystems supply struct dirent d_type field, in which case > everything in the specified directory would go ignored. One such > filesystem being XFS which RHEL 7 defaults to... stat() the entries > instead. > > Fixes: 9f8eb1d9ca0f ("eal: support driver loading from directory") > > Signed-off-by: Panu Matilainen <pmatilai@redhat.com> > --- > lib/librte_eal/common/eal_common_options.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_eal/common/eal_common_options.c > b/lib/librte_eal/common/eal_common_options.c > index bed7385..e51fa12 100644 > --- a/lib/librte_eal/common/eal_common_options.c > +++ b/lib/librte_eal/common/eal_common_options.c > @@ -191,12 +191,14 @@ eal_plugindir_init(const char *path) > } > > while ((dent = readdir(d)) != NULL) { > - if (dent->d_type != DT_REG && dent->d_type != DT_LNK) > - continue; > + struct stat sb; > > snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name); > sopath[PATH_MAX-1] = 0; > > + if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode))) > + continue; > + > if (eal_plugin_add(sopath) == -1) > break; > } It looks like you would skip the symbolic links while you were not before. Intended ?
On 11/20/2015 06:38 PM, David Marchand wrote: > Hello Panu, > > On Wed, Nov 18, 2015 at 7:45 AM, Panu Matilainen <pmatilai@redhat.com> > wrote: > >> Not all filesystems supply struct dirent d_type field, in which case >> everything in the specified directory would go ignored. One such >> filesystem being XFS which RHEL 7 defaults to... stat() the entries >> instead. >> >> Fixes: 9f8eb1d9ca0f ("eal: support driver loading from directory") >> >> Signed-off-by: Panu Matilainen <pmatilai@redhat.com> >> --- >> lib/librte_eal/common/eal_common_options.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/lib/librte_eal/common/eal_common_options.c >> b/lib/librte_eal/common/eal_common_options.c >> index bed7385..e51fa12 100644 >> --- a/lib/librte_eal/common/eal_common_options.c >> +++ b/lib/librte_eal/common/eal_common_options.c >> @@ -191,12 +191,14 @@ eal_plugindir_init(const char *path) >> } >> >> while ((dent = readdir(d)) != NULL) { >> - if (dent->d_type != DT_REG && dent->d_type != DT_LNK) >> - continue; >> + struct stat sb; >> >> snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name); >> sopath[PATH_MAX-1] = 0; >> >> + if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode))) >> + continue; >> + >> if (eal_plugin_add(sopath) == -1) >> break; >> } > > > > It looks like you would skip the symbolic links while you were not before. > Intended ? Intended. We want to accept symlinks in the driver directory, but the actual drivers are always regular files. Since stat() dereferences symbolic links it cannot return a file whose type would be S_IFLNK. - Panu -
Hi Panu, 2015-11-23 08:04, Panu Matilainen: > On 11/20/2015 06:38 PM, David Marchand wrote: > > It looks like you would skip the symbolic links while you were not before. > > Intended ? > > Intended. We want to accept symlinks in the driver directory, but the > actual drivers are always regular files. No we use symbolic links: http://dpdk.org/browse/dpdk/commit/mk/rte.lib.mk?id=133b75923 http://dpdk.org/browse/dpdk/tree/mk/rte.lib.mk#n170 On the same topic, you've added a check in 9f8eb1d9: + if (stat(solib->name, &sb) == -1) { + RTE_LOG(ERR, EAL, "Invalid plugin specified: %s: %s\n", + solib->name, strerror(errno)); It is a regression because we cannot anymore load a plugin from LD_LIBRARY_PATH without specifying its full path. It can be tested with scripts/test-null.sh. How do you suggest to fix it? Thanks
On 11/23/2015 12:41 PM, Thomas Monjalon wrote: > Hi Panu, > > 2015-11-23 08:04, Panu Matilainen: >> On 11/20/2015 06:38 PM, David Marchand wrote: >>> It looks like you would skip the symbolic links while you were not before. >>> Intended ? >> >> Intended. We want to accept symlinks in the driver directory, but the >> actual drivers are always regular files. > > No we use symbolic links: > http://dpdk.org/browse/dpdk/commit/mk/rte.lib.mk?id=133b75923 > http://dpdk.org/browse/dpdk/tree/mk/rte.lib.mk#n170 Those symlinks eventually point to the actual drivers which are always regular files (and not for example directories), that is the point. Please apply. > > On the same topic, you've added a check in 9f8eb1d9: > + if (stat(solib->name, &sb) == -1) { > + RTE_LOG(ERR, EAL, "Invalid plugin specified: %s: %s\n", > + solib->name, strerror(errno)); > > It is a regression because we cannot anymore load a plugin from > LD_LIBRARY_PATH without specifying its full path. > It can be tested with scripts/test-null.sh. > How do you suggest to fix it? My first and foremost thought is regret over messing with -d at all, instead of keeping with the original plan of a new -D option for directories. Supporting directories with -d opened up all these annoying little issues that did not exist in the original plan, and lost the ability to disable/override the default plugindir. I suppose the fix is to only use a successful stat to determine if something is a directory and feed everything else to dlopen() which will then fail on its own if file is not existent. I'll send a separate patch for the regression. - Panu - > Thanks >
2015-11-23 13:42, Panu Matilainen: > On 11/23/2015 12:41 PM, Thomas Monjalon wrote: > > 2015-11-23 08:04, Panu Matilainen: > >> On 11/20/2015 06:38 PM, David Marchand wrote: > >>> It looks like you would skip the symbolic links while you were not before. > >>> Intended ? > >> > >> Intended. We want to accept symlinks in the driver directory, but the > >> actual drivers are always regular files. > > > > No we use symbolic links: > > http://dpdk.org/browse/dpdk/commit/mk/rte.lib.mk?id=133b75923 > > http://dpdk.org/browse/dpdk/tree/mk/rte.lib.mk#n170 > > Those symlinks eventually point to the actual drivers which are always > regular files (and not for example directories), that is the point. > Please apply. Yes > > On the same topic, you've added a check in 9f8eb1d9: > > + if (stat(solib->name, &sb) == -1) { > > + RTE_LOG(ERR, EAL, "Invalid plugin specified: %s: %s\n", > > + solib->name, strerror(errno)); > > > > It is a regression because we cannot anymore load a plugin from > > LD_LIBRARY_PATH without specifying its full path. > > It can be tested with scripts/test-null.sh. > > How do you suggest to fix it? > > My first and foremost thought is regret over messing with -d at all, > instead of keeping with the original plan of a new -D option for > directories. Supporting directories with -d opened up all these annoying > little issues that did not exist in the original plan, and lost the > ability to disable/override the default plugindir. I keep thinking having an unique option is a good thing. If disabling the default directory is needed, we can imagine a special syntax like -d- Thanks for your work. > I suppose the fix is to only use a successful stat to determine if > something is a directory and feed everything else to dlopen() which will > then fail on its own if file is not existent. Yes sounds good. > I'll send a separate patch for the regression. Thanks
2015-11-18 08:45, Panu Matilainen: > Not all filesystems supply struct dirent d_type field, in which case > everything in the specified directory would go ignored. One such > filesystem being XFS which RHEL 7 defaults to... stat() the entries > instead. > > Fixes: 9f8eb1d9ca0f ("eal: support driver loading from directory") > > Signed-off-by: Panu Matilainen <pmatilai@redhat.com> Applied, thanks
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c index bed7385..e51fa12 100644 --- a/lib/librte_eal/common/eal_common_options.c +++ b/lib/librte_eal/common/eal_common_options.c @@ -191,12 +191,14 @@ eal_plugindir_init(const char *path) } while ((dent = readdir(d)) != NULL) { - if (dent->d_type != DT_REG && dent->d_type != DT_LNK) - continue; + struct stat sb; snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name); sopath[PATH_MAX-1] = 0; + if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode))) + continue; + if (eal_plugin_add(sopath) == -1) break; }