On 5/8/24 16:43, Thomas Lamprecht wrote:
On 08/05/2024 14:41, Dominik Csapak wrote:
if the fuse tool encounters an error early, it prints it like:
    Error: some error message
on stderr.

We can capture that here by redirecting STDERR to $wr and die'ing with

using just a variable name like $wr without context in a commit message
is hardly telling or useful, maybe replace above and the paragraph below
with something like:

"Redirect the STDERR of the child process that mounts the ESXi instance to
the pipe of the parent (API) process, so that it can pass a hopefully more
meaningful message to the user than just an erroneous return code."



You're right, that sounds better

the error message.

With this we die with the original error message instead of only
with the return code which is telling the user nothing and does not
help us debug.

Signed-off-by: Dominik Csapak <d.csa...@proxmox.com>
---
  src/PVE/Storage/ESXiPlugin.pm | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index b8bce0e..8dc33fc 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -222,6 +222,10 @@ sub esxi_mount : prototype($$$;$) {
                // die "failed to get file descriptor flags: $!\n";
            fcntl($wr, F_SETFD, $flags & ~FD_CLOEXEC)
                // die "failed to remove CLOEXEC flag from fd: $!\n";
+
+           # capture errors from stderr

nit: you capture all that gets printed to stderr, not just errors, and no
hard feelings, but the comment feels a tiny bit superfluous, at least with
the error message.

true, i'll remove the comment


+           open(STDERR, ">&", \*$wr) or die "unable to redirect STDERR: $!\n";

Don't the \ reference operator and the * dereference operator here cancel
each other out?

yes, of course, i'll remove those


+
            # FIXME: use the user/group options!
            exec {$ESXI_FUSE_TOOL}
                $ESXI_FUSE_TOOL,
@@ -245,7 +249,7 @@ sub esxi_mount : prototype($$$;$) {
      undef $wr;
my $result = do { local $/ = undef; <$rd> };
-    if ($result =~ /^ERROR: (.*)$/) {
+    if ($result =~ /^ERROR: (.*)$/i) {
        die "$1\n";
      }




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

Reply via email to