| Message ID | 20220408045743.1432968-3-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: introduce per-memcg proactive reclaim | expand |
On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > Currently, cg_write() returns 0 on success and -1 on failure. Modify it > to return the errno of write() syscall when write() fails. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++--------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index dbaa7aabbb4a..3b6bb09985fa 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len) > return len; > } > > -static ssize_t write_text(const char *path, char *buf, ssize_t len) > +/* > + * Returns: > + * success -> 0 > + * open() failure -> -1 > + * write() failure -> errno > + */ > +static int write_text(const char *path, char *buf, ssize_t len) > { > - int fd; > + int fd, ret; > > fd = open(path, O_WRONLY | O_APPEND); > if (fd < 0) > return fd; > > - len = write(fd, buf, len); > - if (len < 0) { > - close(fd); > - return len; > - } > - > + ret = write(fd, buf, len) < 0 ? errno : 0; > close(fd); > - > - return len; > + return ret; > } > > char *cg_name(const char *root, const char *name) > @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control) > return cnt; > } > > +/* > + * Returns: > + * success -> 0 > + * open() failure -> -1 > + * write() failure -> errno > + */ > int cg_write(const char *cgroup, const char *control, char *buf) > { > char path[PATH_MAX]; > ssize_t len = strlen(buf); > > snprintf(path, sizeof(path), "%s/%s", cgroup, control); > - > - if (write_text(path, buf, len) == len) > - return 0; > - > - return -1; > + return write_text(path, buf, len); > } I have changed this in v4 to a cleaner implementation that either returns 0 on success or -errno on failure. I also made sure to check that the full buffer was being written, and updated cg_read() as well for the interface to be consistent. Will send out once the discussion on patch 1 in v3 reaches a consensus. > > int cg_find_unified_root(char *root, size_t len) > -- > 2.35.1.1178.g4f1659d476-goog >
On Fri, Apr 08, 2022 at 04:57:41AM +0000, Yosry Ahmed wrote: > Currently, cg_write() returns 0 on success and -1 on failure. Modify it > to return the errno of write() syscall when write() fails. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++--------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index dbaa7aabbb4a..3b6bb09985fa 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len) > return len; > } > > -static ssize_t write_text(const char *path, char *buf, ssize_t len) > +/* > + * Returns: > + * success -> 0 > + * open() failure -> -1 Is there a reason why we can't return errno in this case as well? It would be more straightforward. Otherwise the change looks good to me.
On Fri, Apr 08, 2022 at 06:21:55PM -0700, Yosry Ahmed wrote: > On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > Currently, cg_write() returns 0 on success and -1 on failure. Modify it > > to return the errno of write() syscall when write() fails. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++--------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > > index dbaa7aabbb4a..3b6bb09985fa 100644 > > --- a/tools/testing/selftests/cgroup/cgroup_util.c > > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len) > > return len; > > } > > > > -static ssize_t write_text(const char *path, char *buf, ssize_t len) > > +/* > > + * Returns: > > + * success -> 0 > > + * open() failure -> -1 > > + * write() failure -> errno > > + */ > > +static int write_text(const char *path, char *buf, ssize_t len) > > { > > - int fd; > > + int fd, ret; > > > > fd = open(path, O_WRONLY | O_APPEND); > > if (fd < 0) > > return fd; > > > > - len = write(fd, buf, len); > > - if (len < 0) { > > - close(fd); > > - return len; > > - } > > - > > + ret = write(fd, buf, len) < 0 ? errno : 0; > > close(fd); > > - > > - return len; > > + return ret; > > } > > > > char *cg_name(const char *root, const char *name) > > @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control) > > return cnt; > > } > > > > +/* > > + * Returns: > > + * success -> 0 > > + * open() failure -> -1 > > + * write() failure -> errno > > + */ > > int cg_write(const char *cgroup, const char *control, char *buf) > > { > > char path[PATH_MAX]; > > ssize_t len = strlen(buf); > > > > snprintf(path, sizeof(path), "%s/%s", cgroup, control); > > - > > - if (write_text(path, buf, len) == len) > > - return 0; > > - > > - return -1; > > + return write_text(path, buf, len); > > } > > I have changed this in v4 to a cleaner implementation that either > returns 0 on success or -errno on failure. I also made sure to check > that the full buffer was being written, and updated cg_read() as well > for the interface to be consistent. > > Will send out once the discussion on patch 1 in v3 reaches a consensus. Ok, sounds good, please feel free to add my Acked-by: Roman Gushchin <roman.gushchin@linux.dev> for the whole series. It looks really nice and ready for merging upstream to me. Thanks!
On Fri, Apr 8, 2022 at 6:44 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > On Fri, Apr 08, 2022 at 06:21:55PM -0700, Yosry Ahmed wrote: > > On Thu, Apr 7, 2022 at 9:57 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > > > Currently, cg_write() returns 0 on success and -1 on failure. Modify it > > > to return the errno of write() syscall when write() fails. > > > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > > --- > > > tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++--------- > > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > > > index dbaa7aabbb4a..3b6bb09985fa 100644 > > > --- a/tools/testing/selftests/cgroup/cgroup_util.c > > > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > > > @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len) > > > return len; > > > } > > > > > > -static ssize_t write_text(const char *path, char *buf, ssize_t len) > > > +/* > > > + * Returns: > > > + * success -> 0 > > > + * open() failure -> -1 > > > + * write() failure -> errno > > > + */ > > > +static int write_text(const char *path, char *buf, ssize_t len) > > > { > > > - int fd; > > > + int fd, ret; > > > > > > fd = open(path, O_WRONLY | O_APPEND); > > > if (fd < 0) > > > return fd; > > > > > > - len = write(fd, buf, len); > > > - if (len < 0) { > > > - close(fd); > > > - return len; > > > - } > > > - > > > + ret = write(fd, buf, len) < 0 ? errno : 0; > > > close(fd); > > > - > > > - return len; > > > + return ret; > > > } > > > > > > char *cg_name(const char *root, const char *name) > > > @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control) > > > return cnt; > > > } > > > > > > +/* > > > + * Returns: > > > + * success -> 0 > > > + * open() failure -> -1 > > > + * write() failure -> errno > > > + */ > > > int cg_write(const char *cgroup, const char *control, char *buf) > > > { > > > char path[PATH_MAX]; > > > ssize_t len = strlen(buf); > > > > > > snprintf(path, sizeof(path), "%s/%s", cgroup, control); > > > - > > > - if (write_text(path, buf, len) == len) > > > - return 0; > > > - > > > - return -1; > > > + return write_text(path, buf, len); > > > } > > > > I have changed this in v4 to a cleaner implementation that either > > returns 0 on success or -errno on failure. I also made sure to check > > that the full buffer was being written, and updated cg_read() as well > > for the interface to be consistent. > > > > Will send out once the discussion on patch 1 in v3 reaches a consensus. > > Ok, sounds good, please feel free to add my > Acked-by: Roman Gushchin <roman.gushchin@linux.dev> > for the whole series. It looks really nice and ready for merging upstream to me. > > Thanks! Thanks a lot for reviewing it! I would appreciate it if you still took a look at the v4 version of this patch when I send it (probably early next week) as it's significantly different and includes changes to cg_read() and read_text() as well.
diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index dbaa7aabbb4a..3b6bb09985fa 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -38,23 +38,23 @@ static ssize_t read_text(const char *path, char *buf, size_t max_len) return len; } -static ssize_t write_text(const char *path, char *buf, ssize_t len) +/* + * Returns: + * success -> 0 + * open() failure -> -1 + * write() failure -> errno + */ +static int write_text(const char *path, char *buf, ssize_t len) { - int fd; + int fd, ret; fd = open(path, O_WRONLY | O_APPEND); if (fd < 0) return fd; - len = write(fd, buf, len); - if (len < 0) { - close(fd); - return len; - } - + ret = write(fd, buf, len) < 0 ? errno : 0; close(fd); - - return len; + return ret; } char *cg_name(const char *root, const char *name) @@ -177,17 +177,19 @@ long cg_read_lc(const char *cgroup, const char *control) return cnt; } +/* + * Returns: + * success -> 0 + * open() failure -> -1 + * write() failure -> errno + */ int cg_write(const char *cgroup, const char *control, char *buf) { char path[PATH_MAX]; ssize_t len = strlen(buf); snprintf(path, sizeof(path), "%s/%s", cgroup, control); - - if (write_text(path, buf, len) == len) - return 0; - - return -1; + return write_text(path, buf, len); } int cg_find_unified_root(char *root, size_t len)
Currently, cg_write() returns 0 on success and -1 on failure. Modify it to return the errno of write() syscall when write() fails. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 32 +++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-)