diff mbox

[dpdk-dev,2/5] eal: extract function eal_parse_sysfs_valuef

Message ID 1472704915-13112-3-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Shreyansh Jain Sept. 1, 2016, 4:41 a.m. UTC 
  From: Jan Viktorin <viktorin@rehivetech.com>

From: Jan Viktorin <viktorin@rehivetech.com>

The eal_parse_sysfs_value function accepts a filename however, such
interface introduces race-conditions to the code. Introduce the
variant of this function that accepts an already opened file instead of
a filename.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 lib/librte_eal/common/eal_filesystem.h |  5 +++++
 lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

Comments

Stephen Hemminger Sept. 1, 2016, 6:30 a.m. UTC | #1 
On Thu, 1 Sep 2016 10:11:52 +0530
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> From: Jan Viktorin <viktorin@rehivetech.com>
> 
> From: Jan Viktorin <viktorin@rehivetech.com>
> 
> The eal_parse_sysfs_value function accepts a filename however, such
> interface introduces race-conditions to the code. Introduce the
> variant of this function that accepts an already opened file instead of
> a filename.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---

You introduce new API, but don't use it in your other patches.
I don't see where passing filename is racy. sysfs files only get created/destroyed
when device is added/removed.
Shreyansh Jain Sept. 1, 2016, 9:01 a.m. UTC | #2 
Hi Stephen,

On Thursday 01 September 2016 12:00 PM, Stephen Hemminger wrote:
> On Thu, 1 Sep 2016 10:11:52 +0530
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> The eal_parse_sysfs_value function accepts a filename however, such
>> interface introduces race-conditions to the code. Introduce the
>> variant of this function that accepts an already opened file instead of
>> a filename.
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>
> You introduce new API, but don't use it in your other patches.

Indeed - I have not used this API anywhere in my patches. As highlighted 
in the covering letter, these are some proposed changes which *might* 
help in future (for e.g., introducing new SoC infra).
Patches don't depend on each other.

> I don't see where passing filename is racy. sysfs files only get created/destroyed
> when device is added/removed.
>

Agree that 'race-condition' is not the right word.

parse_sysfs_value reads a single integer from a given sysfs file and is 
a useful helper for EAL layer, specifically for non-PCI infra as and 
when it is introduced (reading through custom sysfs files).
At that time, it may be possible that caller keeps the context of the 
call rather than open the file every time - for reading more than an 
integer, for example.

-
Shreyansh
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_filesystem.h b/lib/librte_eal/common/eal_filesystem.h
index fdb4a70..7875454 100644
--- a/lib/librte_eal/common/eal_filesystem.h
+++ b/lib/librte_eal/common/eal_filesystem.h
@@ -43,6 +43,7 @@ 
 /** Path of rte config file. */
 #define RUNTIME_CONFIG_FMT "%s/.%s_config"
 
+#include <stdio.h>
 #include <stdint.h>
 #include <limits.h>
 #include <unistd.h>
@@ -115,4 +116,8 @@  eal_get_hugefile_temp_path(char *buffer, size_t buflen, const char *hugedir, int
  * Used to read information from files on /sys */
 int eal_parse_sysfs_value(const char *filename, unsigned long *val);
 
+/** Function to read a single numeric value from a file on the filesystem.
+ * Used to read information from files on /sys */
+int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
+
 #endif /* EAL_FILESYSTEM_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d5b81a3..f912e4e 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -126,13 +126,30 @@  rte_eal_get_configuration(void)
 	return &rte_config;
 }
 
+int
+eal_parse_sysfs_valuef(FILE *f, unsigned long *val)
+{
+	char buf[BUFSIZ];
+	char *end = NULL;
+
+	RTE_VERIFY(f != NULL);
+
+	if (fgets(buf, sizeof(buf), f) == NULL)
+		return -1;
+
+	*val = strtoul(buf, &end, 0);
+	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n'))
+		return -2;
+
+	return 0;
+}
+
 /* parse a sysfs (or other) file containing one integer value */
 int
 eal_parse_sysfs_value(const char *filename, unsigned long *val)
 {
+	int ret;
 	FILE *f;
-	char buf[BUFSIZ];
-	char *end = NULL;
 
 	if ((f = fopen(filename, "r")) == NULL) {
 		RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
@@ -140,21 +157,18 @@  eal_parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	ret = eal_parse_sysfs_valuef(f, val);
+	if (ret == -1) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
-			__func__, filename);
-		fclose(f);
-		return -1;
+				__func__, filename);
 	}
-	*val = strtoul(buf, &end, 0);
-	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
+	else if (ret < 0) {
 		RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
 				__func__, filename);
-		fclose(f);
-		return -1;
 	}
+
 	fclose(f);
-	return 0;
+	return ret;
 }