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, ...)
-- 
2.48.1

On 25/02/22 05:42PM, ritvikfoss@gmail.com wrote:
> 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, ...)
> -- 
> 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