[1/2] lib: add helper to read strings from sysfs files

Message ID 20230125103311.1249988-2-tduszynski@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series add platform bus |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation warning apply patch failure

Commit Message

Tomasz Duszynski Jan. 25, 2023, 10:33 a.m. UTC
  Reading strings from sysfs files is a re-occurring pattern
hence add helper for doing that.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
---
 app/test/test_eal_fs.c          | 108 ++++++++++++++++++++++++++++----
 lib/eal/common/eal_filesystem.h |   6 ++
 lib/eal/unix/eal_filesystem.c   |  24 ++++---
 lib/eal/version.map             |   1 +
 4 files changed, 121 insertions(+), 18 deletions(-)
  

Comments

Thomas Monjalon Jan. 25, 2023, 10:39 a.m. UTC | #1
25/01/2023 11:33, Tomasz Duszynski:
> Reading strings from sysfs files is a re-occurring pattern
> hence add helper for doing that.

In general it would be to nice to clean sysfs parsing in libs and drivers,
so they all use some functions from EAL.
  
Tyler Retzlaff Jan. 25, 2023, 4:16 p.m. UTC | #2
On Wed, Jan 25, 2023 at 11:39:30AM +0100, Thomas Monjalon wrote:
> 25/01/2023 11:33, Tomasz Duszynski:
> > Reading strings from sysfs files is a re-occurring pattern
> > hence add helper for doing that.
> 
> In general it would be to nice to clean sysfs parsing in libs and drivers,
> so they all use some functions from EAL.

maybe there should be a general utility library for dealing with sysfs
separate from the core EAL that drivers / platform specific libs can
share?
  
Tomasz Duszynski Jan. 26, 2023, 8:30 a.m. UTC | #3
>-----Original Message-----
>From: Tyler Retzlaff <roretzla@linux.microsoft.com>
>Sent: Wednesday, January 25, 2023 5:16 PM
>To: Thomas Monjalon <thomas@monjalon.net>
>Cc: Tomasz Duszynski <tduszynski@marvell.com>; dev@dpdk.org; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; stephen@networkplumber.org; chenbo.xia@intel.com; david.marchand@redhat.com;
>bruce.richardson@intel.com
>Subject: [EXT] Re: [PATCH 1/2] lib: add helper to read strings from sysfs files
>
>External Email
>
>----------------------------------------------------------------------
>On Wed, Jan 25, 2023 at 11:39:30AM +0100, Thomas Monjalon wrote:
>> 25/01/2023 11:33, Tomasz Duszynski:
>> > Reading strings from sysfs files is a re-occurring pattern hence add
>> > helper for doing that.
>>
>> In general it would be to nice to clean sysfs parsing in libs and
>> drivers, so they all use some functions from EAL.
>
>maybe there should be a general utility library for dealing with sysfs separate from the core EAL
>that drivers / platform specific libs can share?

reading/writing of sysfs files is scattered around the codebase and this has been piling up
with each and and every new pmd/lib that requires it. So generally a few simple utility functions 
in one place may be a good idea. 

Would following make sense?

rte_sysfs_write_int()
rte_sysfs_write_string()
rte_sysfs_read_int()
rte_sysfs_read_string() 

Also seems that pattern where file gets opened once and keeps being written to until closed is 
reoccurring as well. So there might be some utils for that as well. Thoughts?
  
Tomasz Duszynski Jan. 26, 2023, 8:35 a.m. UTC | #4
>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Wednesday, January 25, 2023 11:40 AM
>To: Tomasz Duszynski <tduszynski@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; stephen@networkplumber.org;
>chenbo.xia@intel.com; david.marchand@redhat.com; bruce.richardson@intel.com
>Subject: [EXT] Re: [PATCH 1/2] lib: add helper to read strings from sysfs files
>
>External Email
>
>----------------------------------------------------------------------
>25/01/2023 11:33, Tomasz Duszynski:
>> Reading strings from sysfs files is a re-occurring pattern hence add
>> helper for doing that.
>
>In general it would be to nice to clean sysfs parsing in libs and drivers, so they all use some
>functions from EAL.
>

That's generally true. Here I wanted to avoid tree-wide changes caused by unrelated work i.e a new bus
and do a cleanup, i.e use this read string util where applicable, later on.
  
Tyler Retzlaff Jan. 26, 2023, 5:21 p.m. UTC | #5
On Thu, Jan 26, 2023 at 08:30:01AM +0000, Tomasz Duszynski wrote:
> 
> >-----Original Message-----
> >From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >Sent: Wednesday, January 25, 2023 5:16 PM
> >To: Thomas Monjalon <thomas@monjalon.net>
> >Cc: Tomasz Duszynski <tduszynski@marvell.com>; dev@dpdk.org; Jerin Jacob Kollanukkaran
> ><jerinj@marvell.com>; stephen@networkplumber.org; chenbo.xia@intel.com; david.marchand@redhat.com;
> >bruce.richardson@intel.com
> >Subject: [EXT] Re: [PATCH 1/2] lib: add helper to read strings from sysfs files
> >
> >External Email
> >
> >----------------------------------------------------------------------
> >On Wed, Jan 25, 2023 at 11:39:30AM +0100, Thomas Monjalon wrote:
> >> 25/01/2023 11:33, Tomasz Duszynski:
> >> > Reading strings from sysfs files is a re-occurring pattern hence add
> >> > helper for doing that.
> >>
> >> In general it would be to nice to clean sysfs parsing in libs and
> >> drivers, so they all use some functions from EAL.
> >
> >maybe there should be a general utility library for dealing with sysfs separate from the core EAL
> >that drivers / platform specific libs can share?
> 
> reading/writing of sysfs files is scattered around the codebase and this has been piling up
> with each and and every new pmd/lib that requires it. So generally a few simple utility functions 
> in one place may be a good idea. 

i'm an advocate of smaller libraries that tackle a subject area and do
so well. even better if they can be unit tested without dragging in a
lot of dependencies or bootstrapping other unrelated subsystems.

it is also in alignment with trying to de-bloat eal which i think there
is increasing interest in.

> 
> Would following make sense?
> 
> rte_sysfs_write_int()
> rte_sysfs_write_string()
> rte_sysfs_read_int()
> rte_sysfs_read_string() 
> 
> Also seems that pattern where file gets opened once and keeps being written to until closed is 
> reoccurring as well. So there might be some utils for that as well. Thoughts? 

i guess the answer here is whatever makes a simple intuitive api for
sysfs access, i don't contribute much on the linux side to dpdk so can't
speak to what makes a good api here, but i imagine others can in review.

thanks
  

Patch

diff --git a/app/test/test_eal_fs.c b/app/test/test_eal_fs.c
index b3686edcb4..6c373fc7f1 100644
--- a/app/test/test_eal_fs.c
+++ b/app/test/test_eal_fs.c
@@ -20,12 +20,33 @@  test_eal_fs(void)
 
 #else
 
+static int
+temp_create(char *filename, size_t len)
+{
+	char file_template[] = "/tmp/eal_test_XXXXXX";
+	char proc_path[PATH_MAX];
+	int fd;
+
+	fd = mkstemp(file_template);
+	if (fd == -1) {
+		perror("mkstemp() failure");
+		return -1;
+	}
+
+	snprintf(proc_path, sizeof(proc_path), "/proc/self/fd/%d", fd);
+	if (readlink(proc_path, filename, len) < 0) {
+		perror("readlink() failure");
+		close(fd);
+		return -1;
+	}
+
+	return fd;
+}
+
 static int
 test_parse_sysfs_value(void)
 {
 	char filename[PATH_MAX] = "";
-	char proc_path[PATH_MAX];
-	char file_template[] = "/tmp/eal_test_XXXXXX";
 	int tmp_file_handle = -1;
 	FILE *fd = NULL;
 	unsigned valid_number;
@@ -40,16 +61,10 @@  test_parse_sysfs_value(void)
 
 	/* get a temporary filename to use for all tests - create temp file handle and then
 	 * use /proc to get the actual file that we can open */
-	tmp_file_handle = mkstemp(file_template);
-	if (tmp_file_handle == -1) {
-		perror("mkstemp() failure");
+	tmp_file_handle = temp_create(filename, sizeof(filename));
+	if (tmp_file_handle < 0)
 		goto error;
-	}
-	snprintf(proc_path, sizeof(proc_path), "/proc/self/fd/%d", tmp_file_handle);
-	if (readlink(proc_path, filename, sizeof(filename)) < 0) {
-		perror("readlink() failure");
-		goto error;
-	}
+
 	printf("Temporary file is: %s\n", filename);
 
 	/* test we get an error value if we use file before it's created */
@@ -175,11 +190,82 @@  test_parse_sysfs_value(void)
 	return -1;
 }
 
+static int
+test_parse_sysfs_string(void)
+{
+	const char *teststr = "the quick brown dog jumps over the lazy fox\n";
+	char filename[PATH_MAX] = "";
+	char buf[BUFSIZ] = { };
+	int tmp_file_handle;
+	FILE *fd = NULL;
+
+#ifdef RTE_EXEC_ENV_FREEBSD
+	/* BSD doesn't have /proc/pid/fd */
+	return 0;
+#endif
+	printf("Testing function eal_parse_sysfs_string()\n");
+
+	/* get a temporary filename to use for all tests - create temp file handle and then
+	 * use /proc to get the actual file that we can open
+	 */
+	tmp_file_handle = temp_create(filename, sizeof(filename));
+	if (tmp_file_handle < 0)
+		goto error;
+
+	printf("Temporary file is: %s\n", filename);
+
+	/* test we get an error value if we use file before it's created */
+	printf("Test reading a missing file ...\n");
+	if (eal_parse_sysfs_string("/dev/not-quite-null", buf, sizeof(buf)) == 0) {
+		printf("Error with eal_parse_sysfs_string() - returned success on reading empty file\n");
+		goto error;
+	}
+	printf("Confirmed return error when reading empty file\n");
+
+	/* test reading a string from file */
+	printf("Test reading string ...\n");
+	fd = fopen(filename, "w");
+	if (fd == NULL) {
+		printf("line %d, Error opening %s: %s\n", __LINE__, filename, strerror(errno));
+		goto error;
+	}
+	fprintf(fd, "%s", teststr);
+	fclose(fd);
+	fd = NULL;
+	if (eal_parse_sysfs_string(filename, buf, sizeof(buf) - 1) < 0) {
+		printf("eal_parse_sysfs_string() returned error - test failed\n");
+		goto error;
+	}
+	if (strcmp(teststr, buf)) {
+		printf("Invalid string read by eal_parse_sysfs_string() - test failed\n");
+		goto error;
+	}
+	/* don't print newline */
+	buf[strlen(buf) - 1] = '\0';
+	printf("Read '%s\\n' ok\n", buf);
+
+	close(tmp_file_handle);
+	unlink(filename);
+	printf("eal_parse_sysfs_string() - OK\n");
+	return 0;
+
+error:
+	if (fd)
+		fclose(fd);
+	if (tmp_file_handle > 0)
+		close(tmp_file_handle);
+	if (filename[0] != '\0')
+		unlink(filename);
+	return -1;
+}
+
 static int
 test_eal_fs(void)
 {
 	if (test_parse_sysfs_value() < 0)
 		return -1;
+	if (test_parse_sysfs_string() < 0)
+		return -1;
 	return 0;
 }
 
diff --git a/lib/eal/common/eal_filesystem.h b/lib/eal/common/eal_filesystem.h
index 5d21f07c20..ac6449f529 100644
--- a/lib/eal/common/eal_filesystem.h
+++ b/lib/eal/common/eal_filesystem.h
@@ -104,4 +104,10 @@  eal_get_hugefile_path(char *buffer, size_t buflen, const char *hugedir, int f_id
  * Used to read information from files on /sys */
 int eal_parse_sysfs_value(const char *filename, unsigned long *val);
 
+/** Function to read a string from a file on the filesystem.
+ * Used to read information for files in /sys
+ */
+__rte_internal
+int eal_parse_sysfs_string(const char *filename, char *str, size_t size);
+
 #endif /* EAL_FILESYSTEM_H */
diff --git a/lib/eal/unix/eal_filesystem.c b/lib/eal/unix/eal_filesystem.c
index afbab9368a..8ed10094be 100644
--- a/lib/eal/unix/eal_filesystem.c
+++ b/lib/eal/unix/eal_filesystem.c
@@ -76,12 +76,9 @@  int eal_create_runtime_dir(void)
 	return 0;
 }
 
-/* parse a sysfs (or other) file containing one integer value */
-int eal_parse_sysfs_value(const char *filename, unsigned long *val)
+int eal_parse_sysfs_string(const char *filename, char *str, size_t size)
 {
 	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",
@@ -89,19 +86,32 @@  int eal_parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (fgets(str, size, f) == NULL) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
 			__func__, filename);
 		fclose(f);
 		return -1;
 	}
+	fclose(f);
+	return 0;
+}
+
+/* parse a sysfs (or other) file containing one integer value */
+int eal_parse_sysfs_value(const char *filename, unsigned long *val)
+{
+	char buf[BUFSIZ];
+	char *end = NULL;
+	int ret;
+
+	ret = eal_parse_sysfs_string(filename, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
 	*val = strtoul(buf, &end, 0);
 	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
 		RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
 				__func__, filename);
-		fclose(f);
 		return -1;
 	}
-	fclose(f);
 	return 0;
 }
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7dc9..9118bb6228 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -445,6 +445,7 @@  EXPERIMENTAL {
 INTERNAL {
 	global:
 
+	eal_parse_sysfs_string; # WINDOWS_NO_EXPORT
 	rte_bus_register;
 	rte_bus_unregister;
 	rte_eal_get_baseaddr;