From: Ritvik Gupta ritvikfoss@gmail.com
1. Close the file descriptor when write fails. 2. Introduce 'close_or_die' helper function to reduce repetition.
Signed-off-by: Ritvik Gupta ritvikfoss@gmail.com --- .../selftests/mount/unprivileged-remount-test.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mount/unprivileged-remount-test.c b/tools/testing/selftests/mount/unprivileged-remount-test.c index d2917054fe3a..3dd9df58725b 100644 --- a/tools/testing/selftests/mount/unprivileged-remount-test.c +++ b/tools/testing/selftests/mount/unprivileged-remount-test.c @@ -54,6 +54,13 @@ static void die(char *fmt, ...) exit(EXIT_FAILURE); }
+static void close_or_die(char *filename, int fd) { + if (close(fd) != 0) { + die("close of %s failed: %s\n", + filename, strerror(errno)); + } +} + static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list ap) { char buf[4096]; @@ -79,6 +86,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list } written = write(fd, buf, buf_len); if (written != buf_len) { + close_or_die(filename, fd); if (written >= 0) { die("short write to %s\n", filename); } else { @@ -86,10 +94,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list filename, strerror(errno)); } } - if (close(fd) != 0) { - die("close of %s failed: %s\n", - filename, strerror(errno)); - } + close_or_die(filename, fd); }
static void maybe_write_file(char *filename, char *fmt, ...)
From: Ritvik Gupta ritvikfoss@gmail.com
1. Close the file descriptor when write fails. 2. Introduce 'close_or_die' helper function to reduce repetition.
Signed-off-by: Ritvik Gupta ritvikfoss@gmail.com --- Changes in v2: - Fixed formatting
.../selftests/mount/unprivileged-remount-test.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mount/unprivileged-remount-test.c b/tools/testing/selftests/mount/unprivileged-remount-test.c index d2917054fe3a..41d7547c781d 100644 --- a/tools/testing/selftests/mount/unprivileged-remount-test.c +++ b/tools/testing/selftests/mount/unprivileged-remount-test.c @@ -54,6 +54,14 @@ static void die(char *fmt, ...) exit(EXIT_FAILURE); }
+static void close_or_die(char *filename, int fd) +{ + if (close(fd) != 0) { + die("close of %s failed: %s\n", + filename, strerror(errno)); + } +} + static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list ap) { char buf[4096]; @@ -79,6 +87,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list } written = write(fd, buf, buf_len); if (written != buf_len) { + close_or_die(filename, fd); if (written >= 0) { die("short write to %s\n", filename); } else { @@ -86,10 +95,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list filename, strerror(errno)); } } - if (close(fd) != 0) { - die("close of %s failed: %s\n", - filename, strerror(errno)); - } + close_or_die(filename, fd); }
static void maybe_write_file(char *filename, char *fmt, ...)
On 25/02/22 05:42PM, ritvikfoss@gmail.com wrote:
From: Ritvik Gupta ritvikfoss@gmail.com
- Close the file descriptor when write fails.
- Introduce 'close_or_die' helper function to
reduce repetition.
Signed-off-by: Ritvik Gupta ritvikfoss@gmail.com
Changes in v2: - Fixed formatting
.../selftests/mount/unprivileged-remount-test.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mount/unprivileged-remount-test.c b/tools/testing/selftests/mount/unprivileged-remount-test.c index d2917054fe3a..41d7547c781d 100644 --- a/tools/testing/selftests/mount/unprivileged-remount-test.c +++ b/tools/testing/selftests/mount/unprivileged-remount-test.c @@ -54,6 +54,14 @@ static void die(char *fmt, ...) exit(EXIT_FAILURE); } +static void close_or_die(char *filename, int fd) +{
- if (close(fd) != 0) {
die("close of %s failed: %s\n",filename, strerror(errno));- }
+}
static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list ap) { char buf[4096]; @@ -79,6 +87,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list } written = write(fd, buf, buf_len); if (written != buf_len) {
close_or_die(filename, fd);@@ -86,10 +95,7 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list filename, strerror(errno)); } }
- if (close(fd) != 0) {
die("close of %s failed: %s\n",filename, strerror(errno));- }
- close_or_die(filename, fd);
} static void maybe_write_file(char *filename, char *fmt, ...) -- 2.48.1
Closing a file right before a process exits is redundant, since the kernel will clean it up automatically anyway. That said, whether doing this as a best practice is arguable.
Cheers, Seyediman
Yes, the kernel will handle the 'fd' cleanup automatically, but the existing implementation already closes it before exiting. However, in case where write fails, its unhandled. This patch addresses that gap :)
Nevertheless it's subjective indeed. Thanks for reviewing!
Regards Ritvik
On 25/02/23 09:51AM, Ritvik Gupta wrote:
Yes, the kernel will handle the 'fd' cleanup automatically, but the existing implementation already closes it before exiting. However, in case where write fails, its unhandled. This patch addresses that gap :)
Nevertheless it's subjective indeed. Thanks for reviewing!
Regards Ritvik
The current implementation doesn't close the fd before calling the die() function. It only closes fd before returning because vmaybe_write_file() doesn't necessarily exit the process. It only exits in failure cases, including failures when closing the file itself. I think the close() failure path might have caused some confusion.
Cheers, Seyediman
Yes, the kernel will handle the 'fd' cleanup automatically, but the existing implementation already closes it before exiting.
^^^^^^^ Whoops! I meant 'returning' there. Wording issue on my part :P
We're referring to the same thing! Thanks for detailed response :)
Regards Ritvik
linux-kselftest-mirror@lists.linaro.org
-
Ritvik Gupta -
ritvikfoss＠gmail.com
-
Seyediman Seyedarab