[dpdk-dev] [PATCH v3 2/2] eal/linux: Add support for handling built-in kernel modules
W dniu 08.12.2015 o 08:25, Panu Matilainen pisze:
> On 12/07/2015 10:55 PM, Stephen Hemminger wrote:
>> On Mon, 7 Dec 2015 19:36:05 +0100
>> Kamil Rytarowski wrote:
>>
>>> +/* Check if there is sysfs mounted */
>>> +if (stat("/sys/module", ) != 0) {
>>> +RTE_LOG(DEBUG, EAL, "Open /sys/module failed: %s\n",
>>> +strerror(errno));
>>> return -1;
>>> }
>>
>> This check is useless.
>> If /sys/module does not exist then /sys/module/XXX won't exist either.
>
> Yes, but non-mounted sysfs is an error whereas /sys/module/XXX is
> merely an existence test, and the current sole caller in
> pci_vfio_enable() even bothers checking for the difference. So its
> perhaps a bit academic but its not incorrect.
>
> At any rate, the debug messages are incorrect/misleading. It's
> certainly not trying to *open* these directories so it should not
> claim to do so.
>
Yes, this check is to determine whether there is sysfs mounted. It's
different than checking if there is a module loaded.
This seems academical, but it retains the original behavior.
I will try to improve the logging.
> - Panu -
>
>
>
[dpdk-dev] [PATCH v3 2/2] eal/linux: Add support for handling built-in kernel modules
On 12/07/2015 10:55 PM, Stephen Hemminger wrote:
> On Mon, 7 Dec 2015 19:36:05 +0100
> Kamil Rytarowski wrote:
>
>> +/* Check if there is sysfs mounted */
>> +if (stat("/sys/module", ) != 0) {
>> +RTE_LOG(DEBUG, EAL, "Open /sys/module failed: %s\n",
>> +strerror(errno));
>> return -1;
>> }
>
> This check is useless.
> If /sys/module does not exist then /sys/module/XXX won't exist either.
Yes, but non-mounted sysfs is an error whereas /sys/module/XXX is merely
an existence test, and the current sole caller in pci_vfio_enable() even
bothers checking for the difference. So its perhaps a bit academic but
its not incorrect.
At any rate, the debug messages are incorrect/misleading. It's certainly
not trying to *open* these directories so it should not claim to do so.
- Panu -
[dpdk-dev] [PATCH v3 2/2] eal/linux: Add support for handling built-in kernel modules
On Mon, Dec 7, 2015 at 7:36 PM, Kamil Rytarowski <
Kamil.Rytarowski at caviumnetworks.com> wrote:
> Currently rte_eal_check_module() detects Linux kernel modules via reading
> /proc/modules. Built-in ones aren't listed there and therefore they are not
> being found by the script.
>
> Add support for checking built-in modules with parsing the sysfs files
>
> This commit obsoletes the /proc/modules parsing approach.
>
> Signed-off-by: Kamil Rytarowski
> ---
> lib/librte_eal/linuxapp/eal/eal.c | 32 +---
> 1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 635ec36..539188f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -52,6 +52,8 @@
> #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
> #include
> #endif
> +#include
> +#include
>
> #include
> #include
> @@ -901,27 +903,27 @@ int rte_eal_has_hugepages(void)
> int
> rte_eal_check_module(const char *module_name)
> {
> - char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> - int ret = 0;
> - int n;
> + char sysfs_mod_name[PATH_MAX];
> + struct stat st;
>
> if (NULL == module_name)
> return -1;
>
> - FILE *fd = fopen("/proc/modules", "r");
> - if (NULL == fd) {
> - RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> - " error %i (%s)\n", errno, strerror(errno));
> + /* Check if there is sysfs mounted */
> + if (stat("/sys/module", ) != 0) {
> + RTE_LOG(DEBUG, EAL, "Open /sys/module failed: %s\n",
> + strerror(errno));
> return -1;
> }
>
Not sure making a difference between /sys/module and /sys/module/XXX
presence really helps, but this is the same philosophy as how it was done
before...
So, ok let's go with this.
> - while (!feof(fd)) {
> - n = fscanf(fd, "%29s %*[^\n]", mod_name);
> - if ((n == 1) && !strcmp(mod_name, module_name)) {
> - ret = 1;
> - break;
> - }
> +
> + /* A module might be built-in, therefore try sysfs */
> + snprintf(sysfs_mod_name, PATH_MAX, "/sys/module/%s", module_name);
> + if (stat(sysfs_mod_name, ) != 0) {
> + RTE_LOG(DEBUG, EAL, "Open %s failed! error %i (%s)\n",
> + sysfs_mod_name, errno, strerror(errno));
> + return 0;
> }
>
Please, add a check on snprintf return value.
Even if this can't happen, if snprintf fails, stat would access a truncated
or uninitialized (when < 0) path.
With this fixed, you can add my ack.
Thanks.
--
David Marchand
[dpdk-dev] [PATCH v3 2/2] eal/linux: Add support for handling built-in kernel modules
Currently rte_eal_check_module() detects Linux kernel modules via reading
/proc/modules. Built-in ones aren't listed there and therefore they are not
being found by the script.
Add support for checking built-in modules with parsing the sysfs files
This commit obsoletes the /proc/modules parsing approach.
Signed-off-by: Kamil Rytarowski
---
lib/librte_eal/linuxapp/eal/eal.c | 32 +---
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c
b/lib/librte_eal/linuxapp/eal/eal.c
index 635ec36..539188f 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -52,6 +52,8 @@
#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
#include
#endif
+#include
+#include
#include
#include
@@ -901,27 +903,27 @@ int rte_eal_has_hugepages(void)
int
rte_eal_check_module(const char *module_name)
{
- char mod_name[30]; /* Any module names can be longer than 30 bytes? */
- int ret = 0;
- int n;
+ char sysfs_mod_name[PATH_MAX];
+ struct stat st;
if (NULL == module_name)
return -1;
- FILE *fd = fopen("/proc/modules", "r");
- if (NULL == fd) {
- RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
- " error %i (%s)\n", errno, strerror(errno));
+ /* Check if there is sysfs mounted */
+ if (stat("/sys/module", ) != 0) {
+ RTE_LOG(DEBUG, EAL, "Open /sys/module failed: %s\n",
+ strerror(errno));
return -1;
}
- while (!feof(fd)) {
- n = fscanf(fd, "%29s %*[^\n]", mod_name);
- if ((n == 1) && !strcmp(mod_name, module_name)) {
- ret = 1;
- break;
- }
+
+ /* A module might be built-in, therefore try sysfs */
+ snprintf(sysfs_mod_name, PATH_MAX, "/sys/module/%s", module_name);
+ if (stat(sysfs_mod_name, ) != 0) {
+ RTE_LOG(DEBUG, EAL, "Open %s failed! error %i (%s)\n",
+ sysfs_mod_name, errno, strerror(errno));
+ return 0;
}
- fclose(fd);
- return ret;
+ /* Module has been found */
+ return 1;
}
--
2.5.0
[dpdk-dev] [PATCH v3 2/2] eal/linux: Add support for handling built-in kernel modules
On Mon, 7 Dec 2015 19:36:05 +0100
Kamil Rytarowski wrote:
> + /* Check if there is sysfs mounted */
> + if (stat("/sys/module", ) != 0) {
> + RTE_LOG(DEBUG, EAL, "Open /sys/module failed: %s\n",
> + strerror(errno));
> return -1;
> }
This check is useless.
If /sys/module does not exist then /sys/module/XXX won't exist either.
