[PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Chris Wilson]

Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
but the man page leaves the question of errno and signal handling in
doubt. It is documented as returning a negative value and setting
ferror(), but we have been interpreting errno to handle signal
restarting. As that is in doubt, reduce it to a sprintf and reuse our
common interrupt handling write() that already returns -errno.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
---
 lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 8efe889be..b39da4c2a 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
 
 int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
 {
-	FILE *file;
-	int fd;
-	int ret = -1;
+	char stack[128], *buf = stack;
+	va_list tmp;
+	int ret, fd;
 
 	fd = openat(dir, attr, O_WRONLY);
 	if (fd < 0)
-		return -1;
+		return -errno;
 
-	file = fdopen(fd, "w");
-	if (file) {
-		do {
-			ret = vfprintf(file, fmt, ap);
-		} while (ret == -1 && errno == EINTR);
-		fclose(file);
+	va_copy(tmp, ap);
+	ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
+	va_end(tmp);
+	if (ret < 0)
+		return -EINVAL;
+
+	if (ret > sizeof(stack)) {
+		int len = ret + 1;
+
+		buf = malloc(len);
+		if (!buf)
+			return -ENOMEM;
+
+		ret = vsnprintf(buf, ret, fmt, ap);
+		if (ret > len) {
+			free(buf);
+			return -EINVAL;
+		}
 	}
-	close(fd);
+
+	ret = writeN(fd, buf, ret);
+	if (buf != stack)
+		free(buf);
 
 	return ret;
 }
-- 
2.19.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Chris Wilson]

Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
but the man page leaves the question of errno and signal handling in
doubt. It is documented as returning a negative value and setting
ferror(), but we have been interpreting errno to handle signal
restarting. As that is in doubt, reduce it to a sprintf and reuse our
common interrupt handling write() that already returns -errno.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
---
 lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 8efe889be..b39da4c2a 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
 
 int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
 {
-	FILE *file;
-	int fd;
-	int ret = -1;
+	char stack[128], *buf = stack;
+	va_list tmp;
+	int ret, fd;
 
 	fd = openat(dir, attr, O_WRONLY);
 	if (fd < 0)
-		return -1;
+		return -errno;
 
-	file = fdopen(fd, "w");
-	if (file) {
-		do {
-			ret = vfprintf(file, fmt, ap);
-		} while (ret == -1 && errno == EINTR);
-		fclose(file);
+	va_copy(tmp, ap);
+	ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
+	va_end(tmp);
+	if (ret < 0)
+		return -EINVAL;
+
+	if (ret > sizeof(stack)) {
+		int len = ret + 1;
+
+		buf = malloc(len);
+		if (!buf)
+			return -ENOMEM;
+
+		ret = vsnprintf(buf, ret, fmt, ap);
+		if (ret > len) {
+			free(buf);
+			return -EINVAL;
+		}
 	}
-	close(fd);
+
+	ret = writeN(fd, buf, ret);
+	if (buf != stack)
+		free(buf);
 
 	return ret;
 }
-- 
2.19.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Chris Wilson]

Quoting Chris Wilson (2018-08-30 09:44:33)
> Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
> but the man page leaves the question of errno and signal handling in
> doubt. It is documented as returning a negative value and setting
> ferror(), but we have been interpreting errno to handle signal
> restarting. As that is in doubt, reduce it to a sprintf and reuse our
> common interrupt handling write() that already returns -errno.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> ---
>  lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 8efe889be..b39da4c2a 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>  
>  int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  {
> -       FILE *file;
> -       int fd;
> -       int ret = -1;
> +       char stack[128], *buf = stack;
> +       va_list tmp;
> +       int ret, fd;
>  
>         fd = openat(dir, attr, O_WRONLY);
>         if (fd < 0)
> -               return -1;
> +               return -errno;
>  
> -       file = fdopen(fd, "w");
> -       if (file) {
> -               do {
> -                       ret = vfprintf(file, fmt, ap);
> -               } while (ret == -1 && errno == EINTR);
> -               fclose(file);
> +       va_copy(tmp, ap);
> +       ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
> +       va_end(tmp);
> +       if (ret < 0)
> +               return -EINVAL;
> +
> +       if (ret > sizeof(stack)) {
> +               int len = ret + 1;
> +
> +               buf = malloc(len);
> +               if (!buf)
> +                       return -ENOMEM;
> +
> +               ret = vsnprintf(buf, ret, fmt, ap);
> +               if (ret > len) {
> +                       free(buf);
> +                       return -EINVAL;
> +               }
>         }
> -       close(fd);
> +
> +       ret = writeN(fd, buf, ret);
> +       if (buf != stack)
> +               free(buf);

+ close(fd);

>  
>         return ret;
>  }
> -- 
> 2.19.0.rc1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Chris Wilson]

Quoting Chris Wilson (2018-08-30 09:44:33)
> Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
> but the man page leaves the question of errno and signal handling in
> doubt. It is documented as returning a negative value and setting
> ferror(), but we have been interpreting errno to handle signal
> restarting. As that is in doubt, reduce it to a sprintf and reuse our
> common interrupt handling write() that already returns -errno.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> ---
>  lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 8efe889be..b39da4c2a 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>  
>  int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  {
> -       FILE *file;
> -       int fd;
> -       int ret = -1;
> +       char stack[128], *buf = stack;
> +       va_list tmp;
> +       int ret, fd;
>  
>         fd = openat(dir, attr, O_WRONLY);
>         if (fd < 0)
> -               return -1;
> +               return -errno;
>  
> -       file = fdopen(fd, "w");
> -       if (file) {
> -               do {
> -                       ret = vfprintf(file, fmt, ap);
> -               } while (ret == -1 && errno == EINTR);
> -               fclose(file);
> +       va_copy(tmp, ap);
> +       ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
> +       va_end(tmp);
> +       if (ret < 0)
> +               return -EINVAL;
> +
> +       if (ret > sizeof(stack)) {
> +               int len = ret + 1;
> +
> +               buf = malloc(len);
> +               if (!buf)
> +                       return -ENOMEM;
> +
> +               ret = vsnprintf(buf, ret, fmt, ap);
> +               if (ret > len) {
> +                       free(buf);
> +                       return -EINVAL;
> +               }
>         }
> -       close(fd);
> +
> +       ret = writeN(fd, buf, ret);
> +       if (buf != stack)
> +               free(buf);

+ close(fd);

>  
>         return ret;
>  }
> -- 
> 2.19.0.rc1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Katarzyna Dec]

On Thu, Aug 30, 2018 at 09:44:33AM +0100, Chris Wilson wrote:
> Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
> but the man page leaves the question of errno and signal handling in
> doubt. It is documented as returning a negative value and setting
> ferror(), but we have been interpreting errno to handle signal
> restarting. As that is in doubt, reduce it to a sprintf and reuse our
> common interrupt handling write() that already returns -errno.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> ---
>  lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 8efe889be..b39da4c2a 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>  
>  int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  {
> -	FILE *file;
> -	int fd;
> -	int ret = -1;
> +	char stack[128], *buf = stack;
> +	va_list tmp;
> +	int ret, fd;
>  
>  	fd = openat(dir, attr, O_WRONLY);
>  	if (fd < 0)
> -		return -1;
> +		return -errno;
>  
> -	file = fdopen(fd, "w");
> -	if (file) {
> -		do {
> -			ret = vfprintf(file, fmt, ap);
> -		} while (ret == -1 && errno == EINTR);
> -		fclose(file);
> +	va_copy(tmp, ap);
> +	ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
> +	va_end(tmp);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	if (ret > sizeof(stack)) {
> +		int len = ret + 1;
> +
> +		buf = malloc(len);
> +		if (!buf)
> +			return -ENOMEM;
> +
> +		ret = vsnprintf(buf, ret, fmt, ap);
> +		if (ret > len) {
> +			free(buf);
> +			return -EINVAL;
> +		}
>  	}
> -	close(fd);
> +
> +	ret = writeN(fd, buf, ret);
> +	if (buf != stack)
> +		free(buf);
>  
>  	return ret;
>  }

It looks like my issue with returning error from igt_sysfs_printf is solved.
Reviewed-by: Katarzyna Dec <katarzyna.dec@intel.com>

Thanks!
Kasia :)
> -- 
> 2.19.0.rc1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Katarzyna Dec]

On Thu, Aug 30, 2018 at 09:44:33AM +0100, Chris Wilson wrote:
> Currently we wrap our fd inside a FILE* stream to make use of vfprintf,
> but the man page leaves the question of errno and signal handling in
> doubt. It is documented as returning a negative value and setting
> ferror(), but we have been interpreting errno to handle signal
> restarting. As that is in doubt, reduce it to a sprintf and reuse our
> common interrupt handling write() that already returns -errno.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> ---
>  lib/igt_sysfs.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 8efe889be..b39da4c2a 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -387,22 +387,37 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>  
>  int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  {
> -	FILE *file;
> -	int fd;
> -	int ret = -1;
> +	char stack[128], *buf = stack;
> +	va_list tmp;
> +	int ret, fd;
>  
>  	fd = openat(dir, attr, O_WRONLY);
>  	if (fd < 0)
> -		return -1;
> +		return -errno;
>  
> -	file = fdopen(fd, "w");
> -	if (file) {
> -		do {
> -			ret = vfprintf(file, fmt, ap);
> -		} while (ret == -1 && errno == EINTR);
> -		fclose(file);
> +	va_copy(tmp, ap);
> +	ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
> +	va_end(tmp);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	if (ret > sizeof(stack)) {
> +		int len = ret + 1;
> +
> +		buf = malloc(len);
> +		if (!buf)
> +			return -ENOMEM;
> +
> +		ret = vsnprintf(buf, ret, fmt, ap);
> +		if (ret > len) {
> +			free(buf);
> +			return -EINVAL;
> +		}
>  	}
> -	close(fd);
> +
> +	ret = writeN(fd, buf, ret);
> +	if (buf != stack)
> +		free(buf);
>  
>  	return ret;
>  }

It looks like my issue with returning error from igt_sysfs_printf is solved.
Reviewed-by: Katarzyna Dec <katarzyna.dec@intel.com>

Thanks!
Kasia :)
> -- 
> 2.19.0.rc1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Patchwork]

== Series Details ==

Series: lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf
URL   : https://patchwork.freedesktop.org/series/48930/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4737 -> IGTPW_1759 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48930/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1759 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103191, fdo#107362) +1

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (53 -> 49) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


== Build changes ==

    * IGT: IGT_4611 -> IGTPW_1759

  CI_DRM_4737: c8fe77635992b1f6b32102657faced7c9165c942 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1759: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1759/
  IGT_4611: b966dd93a30f41581fe1dbf9bc1c4a29b552ca05 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1759/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf

[Patchwork]

== Series Details ==

Series: lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf
URL   : https://patchwork.freedesktop.org/series/48930/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4611_full -> IGTPW_1759_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1759_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1759_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48930/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1759_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in IGTPW_1759_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    {igt@gem_userptr_blits@readonly-unsync}:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-mmap-cpu:
      shard-glk:          PASS -> FAIL (fdo#103167) +1

    igt@kms_plane@plane-position-covered-pipe-c-planes:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +3

    igt@prime_busy@hang-bsd:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@testdisplay:
      shard-glk:          PASS -> INCOMPLETE (fdo#107093, fdo#103359, k.org#198133)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@gem_render_copy_redux@normal:
      shard-kbl:          INCOMPLETE (fdo#106650, fdo#103665) -> PASS

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-kbl:          INCOMPLETE (fdo#107556, fdo#103665) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-snb:          FAIL (fdo#103925) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106650 https://bugs.freedesktop.org/show_bug.cgi?id=106650
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107093 https://bugs.freedesktop.org/show_bug.cgi?id=107093
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4611 -> IGTPW_1759
    * Linux: CI_DRM_4715 -> CI_DRM_4737

  CI_DRM_4715: 1b73a69651beab39192502181c83e77a1022014a @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4737: c8fe77635992b1f6b32102657faced7c9165c942 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1759: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1759/
  IGT_4611: b966dd93a30f41581fe1dbf9bc1c4a29b552ca05 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1759/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev