Patchwork [i-g-t,v2] lib/igt_sysfs: Let igt_sysfs_read|write return -errno

login
register
mail settings
Submitter Michal Wajdeczko
Date Dec. 7, 2017, 4:52 p.m.
Message ID <20171207165246.17944-1-michal.wajdeczko@intel.com>
Download mbox | patch
Permalink /patch/10100123/
State New
Headers show

Comments

Michal Wajdeczko - Dec. 7, 2017, 4:52 p.m.
In some cases debugfs or sysfs may return errors that we
want to check. Return -errno from helper functions to make
asserts easier.

v2: don't forget about EOF ret=0 (Chris)
    small re-write (Michal)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_sysfs.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)
Daniel Vetter - Dec. 7, 2017, 9 p.m.
On Thu, Dec 07, 2017 at 04:59:36PM +0000, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2017-12-07 16:52:46)
> > In some cases debugfs or sysfs may return errors that we
> > want to check. Return -errno from helper functions to make
> > asserts easier.
> > 
> > v2: don't forget about EOF ret=0 (Chris)
> >     small re-write (Michal)
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Just tests/drv_hangman.c needs to fixed to use
> igt_require(sysfs >= 0);

Isn't the usual igt pattern that igt_foo never fails, and __igt_foo gives
you the error code? But I guess we can do that later on in follow-ups.
-Daniel
Michal Wajdeczko - Dec. 13, 2017, 2:16 p.m.
On Thu, 07 Dec 2017 22:00:48 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Dec 07, 2017 at 04:59:36PM +0000, Chris Wilson wrote:
>> Quoting Michal Wajdeczko (2017-12-07 16:52:46)
>> > In some cases debugfs or sysfs may return errors that we
>> > want to check. Return -errno from helper functions to make
>> > asserts easier.
>> >
>> > v2: don't forget about EOF ret=0 (Chris)
>> >     small re-write (Michal)
>> >
>> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Just tests/drv_hangman.c needs to fixed to use
>> igt_require(sysfs >= 0);
>
> Isn't the usual igt pattern that igt_foo never fails, and __igt_foo gives
> you the error code? But I guess we can do that later on in follow-ups.

I'm not sure that this pattern is applicable here as igt_sysfs_read() was
already returning some data or error indicator. The only change is that
now we return len/-errno (instead len/-1)

Michal
Chris Wilson - Dec. 15, 2017, 11:28 a.m.
Quoting Chris Wilson (2017-12-07 16:59:36)
> Quoting Michal Wajdeczko (2017-12-07 16:52:46)
> > In some cases debugfs or sysfs may return errors that we
> > want to check. Return -errno from helper functions to make
> > asserts easier.
> > 
> > v2: don't forget about EOF ret=0 (Chris)
> >     small re-write (Michal)
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And pushed.
-Chris

Patch

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index e7c67da..842a136 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -54,36 +54,34 @@ 
 
 static int readN(int fd, char *buf, int len)
 {
-	int total = 0;
+	int ret, total = 0;
 	do {
-		int ret = read(fd, buf + total, len - total);
-		if (ret < 0 && (errno == EINTR || errno == EAGAIN))
+		ret = read(fd, buf + total, len - total);
+		if (ret < 0)
+			ret = -errno;
+		if (ret == -EINTR || ret == -EAGAIN)
 			continue;
-
 		if (ret <= 0)
-			return total ?: ret;
-
+			break;
 		total += ret;
-		if (total == len)
-			return total;
-	} while (1);
+	} while (total != len);
+	return total ?: ret;
 }
 
 static int writeN(int fd, const char *buf, int len)
 {
-	int total = 0;
+	int ret, total = 0;
 	do {
-		int ret = write(fd, buf + total, len - total);
-		if (ret < 0 && (errno == EINTR || errno == EAGAIN))
+		ret = write(fd, buf + total, len - total);
+		if (ret < 0)
+			ret = -errno;
+		if (ret == -EINTR || ret == -EAGAIN)
 			continue;
-
 		if (ret <= 0)
-			return total ?: ret;
-
+			break;
 		total += ret;
-		if (total == len)
-			return total;
-	} while (1);
+	} while (total != len);
+	return total ?: ret;
 }
 
 /**
@@ -238,7 +236,7 @@  int igt_sysfs_open_parameters(int device)
  * This writes @len bytes from @data to the sysfs file.
  *
  * Returns:
- * The number of bytes written, or -1 on error.
+ * The number of bytes written, or -errno on error.
  */
 int igt_sysfs_write(int dir, const char *attr, const void *data, int len)
 {
@@ -246,7 +244,7 @@  int igt_sysfs_write(int dir, const char *attr, const void *data, int len)
 
 	fd = openat(dir, attr, O_WRONLY);
 	if (fd < 0)
-		return false;
+		return -errno;
 
 	len = writeN(fd, data, len);
 	close(fd);
@@ -264,7 +262,7 @@  int igt_sysfs_write(int dir, const char *attr, const void *data, int len)
  * This reads @len bytes from the sysfs file to @data
  *
  * Returns:
- * The length read, -1 on failure.
+ * The length read, -errno on failure.
  */
 int igt_sysfs_read(int dir, const char *attr, void *data, int len)
 {
@@ -272,7 +270,7 @@  int igt_sysfs_read(int dir, const char *attr, void *data, int len)
 
 	fd = openat(dir, attr, O_RDONLY);
 	if (fd < 0)
-		return false;
+		return -errno;
 
 	len = readN(fd, data, len);
 	close(fd);
@@ -338,7 +336,7 @@  char *igt_sysfs_get(int dir, const char *attr)
 		rem = len - offset - 1;
 	}
 
-	if (ret != -1)
+	if (ret > 0)
 		offset += ret;
 	buf[offset] = '\0';
 	while (offset > 0 && buf[offset-1] == '\n')