Re: [PATCH i-g-t v2 1/4] runner/executor: Check for error at writing dmesg dump

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Hi,

I think that the handling of expected errors such as handling of
EINTR, and partial reads can be improved. I would argue to either
ignore the errors or handle them properly. Anything in between is
likely just creating problem for future selves. Here is my
proposal:

do
	w = write(outfd, buf, r);
while (w == -1 && errno == EINTR);
if (w < r) {
	if (w > 0) {
		ssize_t total_written = w;

		while (total_written < r) {
			ssize_t w_partial = write(outfd, buf + total_written, r - total_written);
			if (w_partial > 0)
				total_written += w_partial;
			else if (w_partial == -1 && errno == EINTR)
				continue; /* Retry */
			else {
				long err = (w_partial == -1) ? -errno : -1;
				errf("Write error while processing dmesg, errno=%d (%ld bytes written out of %ld)\n", errno, total_written, r);
				if (comparefd >= 0)
					close(comparefd);
				return err;
			}
		}
	} else {
		/* Write comnpletely failed */
		errf("Write error while processing dmesg, errno=%d\n", errno);
		if (comparefd >= 0)
			close(comparefd);
		return errno;
	}
}
written += r;

Notice that the expected errors are expected to fail more than
once, and that I tried to organize the code in way that feels
"to me" easier to follow by nesting if (w < r) and if (w > 0).
Also notice that it can block due to EINTR for ever.

While I compile tested this code, I wrote it to ask you what do
we really want to do?
 0 - Forget about write errors until we face issues in real world?
 1 - Get arbitrary about which write errors we want to deal with?
 2 - Make an effort to make a bullet proof error checking for write?

As I mentioned earlier, I am slightly more inclined to do nothing,
but if that is off limits, I see no other path than 2.

Thanks,

Peter

On 20.11.2024 19:32, Kamil Konieczny wrote:
> In processing kernel dmesg there are checks for error at reading
> so add also one for writing.
> 
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  runner/executor.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index ac73e1dde..e4d1fc323 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -600,7 +600,7 @@ static long dump_dmesg(int kmsgfd, int outfd)
>  	bool underflow_once = false;
>  	char cont;
>  	char buf[2048];
> -	ssize_t r;
> +	ssize_t r, w;
>  	long written = 0;
>  
>  	if (kmsgfd < 0)
> @@ -654,9 +654,32 @@ static long dump_dmesg(int kmsgfd, int outfd)
>  			return written;
>  		}
>  
> -		write(outfd, buf, r);
> -		written += r;
> +		w = write(outfd, buf, r);
> +		if (w < r) {
> +			if (w == -1 && errno == EINTR)
> +				w = write(outfd, buf, r);
> +
> +			if (w < r && w >= 0)
> +				w += write(outfd, buf + w, r - w);
> +
> +			if (w < r) {
> +				long err = -errno;
> +
> +				if (err) {
> +					errf("Write error while processing dmesg, errno=%d %ld < %ld\n", errno, w, r);
> +				} else {
> +					errf("Cannot write dmesg chunk: %ld < %ld\n", w, r);
> +					err = -1;
> +				}
>  
> +				if (comparefd >= 0)
> +					close(comparefd);
> +
> +				return err;
> +			}
> +		}
> +
> +		written += r;
>  		if (comparefd < 0 && sscanf(buf, "%u,%llu,%llu,%c;",
>  					    &flags, &seq, &usec, &cont) == 4) {
>  			/*