From: Christoph Heiss <c.heiss@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH common] sysfstools: file_write: properly catch errors
Date: Thu, 17 Oct 2024 12:22:45 +0200 [thread overview]
Message-ID: <tfvglw4ooxd7rmiaj6litl6gkzgfsdu3nsff3ibzl3gd2aog7r@eugcnaxpilzm> (raw)
In-Reply-To: <20240723082925.934603-1-d.csapak@proxmox.com>
Tested this together with the Nvidia vGPU series [0], so please consider
this also:
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
[0] https://lore.proxmox.com/pve-devel/ul3faxwxsi45hk4e44cl2fw4jomoncy53owx3you2wuvo7pymf@dchu53bqfa3p/T/#m2526c3a031aacc2f52080977febd432b4ab2efcf
On Tue, Jul 23, 2024 at 10:29:25AM GMT, Dominik Csapak wrote:
> since `print` is doing buffered IO, we don't always get an error there,
> even if the underlying write does not work.
>
> To properly catch that, do an unbuffered `syswrite` which circumvents
> all buffers and writes directly to the file handle.
>
> We aren't actually interested in the specific error here, but only if
> the write was successful or not.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>
> Note: this is heavily used when doing PCI passthrough, and it seems we
> did not catch as many errors as we thought. Maybe we should introduce an
> 'noerr' parameter and pass that on all current code paths, since i fear
> that there are many situations where the sysfs write had failed but it
> still works because we ignored it and qemu/the kernel do the right thing
> anyway.
Actually catching errors here is definitely the right thing to do, IMHO.
But as for not breaking existing callers, maybe a `warn` statement would
be sensible?
That way broken call-sites can be caught in the future and fixed as
necessary. So a `noerr` parameter and `warn` if that is unset.
>
> src/PVE/SysFSTools.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
> index 57f0ac8..8eb9f2e 100644
> --- a/src/PVE/SysFSTools.pm
> +++ b/src/PVE/SysFSTools.pm
> @@ -198,7 +198,7 @@ sub file_write {
> my $fh = IO::File->new($filename, "w");
> return undef if !$fh;
>
> - my $res = print $fh $buf;
> + my $res = defined(syswrite($fh, $buf)) ? 1 : 0;
>
> $fh->close();
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2024-10-17 10:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-23 8:29 Dominik Csapak
2024-10-17 10:22 ` Christoph Heiss [this message]
2024-10-24 16:32 ` [pve-devel] applied: " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=tfvglw4ooxd7rmiaj6litl6gkzgfsdu3nsff3ibzl3gd2aog7r@eugcnaxpilzm \
--to=c.heiss@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox