Re: [pve-devel] [PATCH common v2 1/2] sysfstools: file_write: extend with logging and ignore list

[Stoiko Ivanov <s.ivanov@proxmox.com>]

2 cosmetic nits inline:
On Fri,  8 Nov 2024 10:32:58 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> the actual error and path is useful to know when trying to debug or
> figure out what did not work, so warn here if there was an error.
> 
> Also takes now an optional error list that can be ignored. If
> encountering such an error, returns success instead of failure.
suggestion:
Now also takes an optional list of errors that can be ignored.

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/SysFSTools.pm | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
> index 0bde6d7..156fee6 100644
> --- a/src/PVE/SysFSTools.pm
> +++ b/src/PVE/SysFSTools.pm
> @@ -211,17 +211,31 @@ sub check_iommu_support{
>      return PVE::Tools::dir_glob_regex('/sys/class/iommu/', "[^\.].*");
>  }
>  
> +# writes $buf into $filename
> +# returns success when encountering an error from the given $ignore_list, e.g. EEXIST
I somehow read this as only condition when it returns success (also not
sure if the comment is needed or this gets clear when looking through the
code)?

maybe:
# writes $buf into $filename, returns false and warns on errors not listed in the optional $ignore_list
?

>  sub file_write {
> -    my ($filename, $buf) = @_;
> +    my ($filename, $buf, $ignore_list) = @_;
if you do:
$ignore_list //= [];
here...

>  
>      my $fh = IO::File->new($filename, "w");
>      return undef if !$fh;
>  
> -    my $res = defined(syswrite($fh, $buf)) ? 1 : 0;
> -
> +    my $res = syswrite($fh, $buf);
>      $fh->close();
>  
> -    return $res;
> +    if (defined($res)) {
> +	return 1;
> +    } elsif (my $err = $!) {
> +	if (defined($ignore_list)) {
.. this nesting can be omitted.
> +	    for my $to_ignore ($ignore_list->@*) {
> +		if ($err == $to_ignore) {
> +		    return 1;
> +		}
the inner if could be shorted a bit to:
return 1 if ($err == $to_ignore);

> +	    }
> +	}
> +	warn "error writing '$buf' to '$filename': $err\n";
> +    }
> +
> +    return 0;
>  }
>  
>  sub pci_device_info {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel