| Message ID | 20180830084433.10035-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] lib/sysfs: Avoid using FILE* temporary for igt_sysfs_[v]printf | expand |
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 >
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 >
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; }
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(-)