std::process::Command is broken on VMWare ESXi about rust HOT 12 OPEN

Yeah, I don't think we could take a straight revert because it would be costing performance on true Linux systems.

Yeah, I understand that. Issue created only for information purposes for users who develop for ESXi target (are there any at all?). Thx y'all for answer!

from rust.

Does ESXi claim that it replicates the functional properties of a Linux kernel of a certain version?

from rust.

Does ESXi claim that it replicates the functional properties of a Linux kernel of a certain version?

No, but ESXi api is very similar to POSIX. I does't found any documentation about certain version.

from rust.

Honestly, Linux is not "very similar to POSIX", so ESXi can't possibly be that similar to Linux if it is very similar to POSIX. Linux is a very specific take on POSIX. It deviates from the standard (or "extends", if you prefer) in many places. I struggle to think differently when most of our Unix code looks like "Linux" and then "Everyone Else". ESXi either has to deviate in the same way or it can't be expected to run our code compiled for Linux.

Linux is Linux, it's not VMware's ESXi, and we cannot coherently support random cut-up jobs of the Linux kernel API. At least when a distro patches the source, we can inspect their patches!

from rust.

Mind, it's not that it's theoretically impossible for us to take on a patch to fix this... but we probably can't fix it ourselves (we would need someone experienced with ESXi to root-cause this, as there's more than just one or two possible failure points in that PR), and we probably will basically never test it for compatibility against ESXi. Not on our own, anyways, because Broadcom will not provide free licenses anymore. So the day after we accept such a patch, we might blithely merge something that breaks ESXi again, and that is because we will test the code works on Linux, not ESXi.

Most of the things I say are mutable, I think, if e.g. Broadcom had a sincere interest in providing sufficient technical expertise to make this work in our CI.

from rust.

I think it could be added a target like a for Windows 7 support ( *-win7-windows-msvc), but you're right - there are few people interested in this.
I fixed it locally but didn't submit a PR since it's unlikely anyone else will need it, including Broadcom.

from rust.

I think esxi-linux as a tier 3 would be on the table, yeah.

I'm curious what the patch is like, at least, unless it's just a revert.

from rust.

I don't have time to look into the matter in more detail, so I just rolled back the changes:

# that patch for std/src/sys/pal/unix/process/process_unix.rs

diff --git a/process_unix.rs b/process_unix.rs
index 011be6a..210d2a9 100644
--- a/process_unix.rs	
+++ b/process_unix.rs
@@ -72,10 +72,6 @@ impl Command {
             return Ok((ret, ours));
         }
 
-        #[cfg(target_os = "linux")]
-        let (input, output) = sys::net::Socket::new_pair(libc::AF_UNIX, libc::SOCK_SEQPACKET)?;
-
-        #[cfg(not(target_os = "linux"))]
         let (input, output) = sys::pipe::anon_pipe()?;
 
         // Whatever happens after the fork is almost for sure going to touch or
@@ -95,10 +91,7 @@ impl Command {
             crate::panic::always_abort();
             mem::forget(env_lock); // avoid non-async-signal-safe unlocking
             drop(input);
-            #[cfg(target_os = "linux")]
-            if self.get_create_pidfd() {
-                self.send_pidfd(&output);
-            }
+
             let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref()) };
             let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32;
             let errno = errno.to_be_bytes();
@@ -122,14 +115,6 @@ impl Command {
         drop(env_lock);
         drop(output);
 
-        #[cfg(target_os = "linux")]
-        let pidfd = if self.get_create_pidfd() {
-            self.recv_pidfd(&input)
-        } else {
-            -1
-        };
-
-        #[cfg(not(target_os = "linux"))]
         let pidfd = -1;
 
         // Safety: We obtained the pidfd (on Linux) using SOCK_SEQPACKET, so it's valid.

And code now work:

// main.rs
use std::process::{Command, Stdio};

fn main() {
    let res = Command::new("ls")
        .arg("-l")
        .stderr(Stdio::null())
        .output()
        .unwrap();
    println!("status = {:?}", res.status);
    println!("stderr = {:?}", String::from_utf8_lossy(&res.stderr));
    println!("stdout = {:?}", String::from_utf8_lossy(&res.stdout));
}

from rust.

That's weird. Those branches should not be taken unless you call command.create_pidfd(true). So given your examples your patch should not make a difference.

from rust.

That's weird. Those branches should not be taken unless you call command.create_pidfd(true). So given your examples your patch should not make a difference.

Yeah, sorry, I post not a full patch. Edited.

from rust.

Ah well, then I guess it doesn't support SOCK_SEQPACKET unix sockets or creating it with SOCK_CLOEXEC.

from rust.

Yeah, I don't think we could take a straight revert because it would be costing performance on true Linux systems.

from rust.