[pbs-devel] [PATCH proxmox 0/2] Use IsTerminal trait

[pbs-devel] [PATCH proxmox 0/2] Use IsTerminal trait

[Maximiliano Sandoval R]

Small patch series that ports code to IsTerminal.

Since the functions stdout_isatty and stdin_isatty are used in multiple projects
(e.g. Backup Server and Offline Mirror), the function were just modified to use
the new trait instead of being removed.

Maximiliano Sandoval R (2):
  sys: Use safe wrapper for libc::isatty
  router: Use safe wrapper for libc::isatty

 proxmox-router/src/cli/text_table.rs |  4 ++--
 proxmox-sys/src/linux/tty.rs         | 20 +++++++-------------
 2 files changed, 9 insertions(+), 15 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH proxmox 1/2] sys: Use safe wrapper for libc::isatty

[Maximiliano Sandoval R]

Use the `std::io::IsTerminal` trait introduced in Rust 1.70.

Internally it calls `libc::isatty`, see [1, 2]. Note that it switches
the comparison from `== 1` to `!= 0` which shouldn't make a difference
assuming that libc::isatty upholds the promises made in its man page.

[1] https://doc.rust-lang.org/src/std/io/stdio.rs.html#1079
[2] https://doc.rust-lang.org/src/std/sys/unix/io.rs.html#79

Signed-off-by: Maximiliano Sandoval R <m.sandoval@proxmox.com>
---
 proxmox-sys/src/linux/tty.rs | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/proxmox-sys/src/linux/tty.rs b/proxmox-sys/src/linux/tty.rs
index fdea162..375e0ec 100644
--- a/proxmox-sys/src/linux/tty.rs
+++ b/proxmox-sys/src/linux/tty.rs
@@ -1,4 +1,4 @@
-use std::io::{self, Read, Write};
+use std::io::{self, IsTerminal, Read, Write};
 use std::mem::MaybeUninit;
 use std::os::unix::io::{AsRawFd, OwnedFd};
 
@@ -25,20 +25,14 @@ pub fn stdout_terminal_size() -> (usize, usize) {
     (winsize.ws_row as usize, winsize.ws_col as usize)
 }
 
-/// Returns whether the current stdout is a tty .
-/// # Safety
-///
-/// uses unsafe call to libc::isatty
+/// Returns whether the current stdout is a tty.
 pub fn stdout_isatty() -> bool {
-    unsafe { libc::isatty(std::io::stdin().as_raw_fd()) == 1 }
+    std::io::stdout().is_terminal()
 }
 
-/// Returns whether the current stdin is a tty .
-/// # Safety
-///
-/// uses unsafe call to libc::isatty
+/// Returns whether the current stdin is a tty.
 pub fn stdin_isatty() -> bool {
-    unsafe { libc::isatty(std::io::stdin().as_raw_fd()) == 1 }
+    std::io::stdin().is_terminal()
 }
 
 pub enum TtyOutput {
@@ -75,7 +69,7 @@ impl TtyOutput {
     /// Get an output file descriptor for the current terminal.
     pub fn open() -> io::Result<Option<Self>> {
         let stdout = std::io::stdout();
-        if unsafe { libc::isatty(stdout.as_raw_fd()) } == 1 {
+        if stdout.is_terminal() {
             Ok(Some(TtyOutput::Stdout(stdout)))
         } else {
             match crate::fd::open(
@@ -97,7 +91,7 @@ impl TtyOutput {
 /// first.
 pub fn read_password(query: &str) -> Result<Vec<u8>, Error> {
     let input = std::io::stdin();
-    if unsafe { libc::isatty(input.as_raw_fd()) } != 1 {
+    if !input.is_terminal() {
         let mut out = String::new();
         input.read_line(&mut out)?;
         return Ok(out.into_bytes());
-- 
2.39.2

[pbs-devel] [PATCH proxmox 2/2] router: Use safe wrapper for libc::isatty

[Maximiliano Sandoval R]

Signed-off-by: Maximiliano Sandoval R <m.sandoval@proxmox.com>
---
 proxmox-router/src/cli/text_table.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-router/src/cli/text_table.rs b/proxmox-router/src/cli/text_table.rs
index 9bc7210..3368605 100644
--- a/proxmox-router/src/cli/text_table.rs
+++ b/proxmox-router/src/cli/text_table.rs
@@ -1,4 +1,4 @@
-use std::io::Write;
+use std::io::{IsTerminal, Write};
 
 use anyhow::{bail, Error};
 use serde_json::Value;
@@ -245,7 +245,7 @@ impl TableFormatOptions {
     pub fn new() -> Self {
         let mut me = Self::default();
 
-        let is_tty = unsafe { libc::isatty(libc::STDOUT_FILENO) == 1 };
+        let is_tty = std::io::stdout().is_terminal();
 
         if is_tty {
             let (_rows, columns) = stdout_terminal_size();
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox 1/2] sys: Use safe wrapper for libc::isatty

[Thomas Lamprecht]

Am 17/11/2023 um 13:37 schrieb Maximiliano Sandoval R:
> Use the `std::io::IsTerminal` trait introduced in Rust 1.70.

ack, but a more general question: should we set the msrv in the manifest:

rust-version = "1.70"

We did not bothered to much with that until now, only a bit indirectly through
increasing the edition to e.g., 2021, and we do not have that many external
users for it to matter much.

Re: [pbs-devel] [PATCH proxmox 1/2] sys: Use safe wrapper for libc::isatty

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 19.11.2023 16:09 CET geschrieben: 
>  
> Am 17/11/2023 um 13:37 schrieb Maximiliano Sandoval R:
> > Use the `std::io::IsTerminal` trait introduced in Rust 1.70.
> 
> ack, but a more general question: should we set the msrv in the manifest:
> 
> rust-version = "1.70"
> 
> We did not bothered to much with that until now, only a bit indirectly through
> increasing the edition to e.g., 2021, and we do not have that many external
> users for it to matter much.

I think that would make sense. most recent debcargo versions (not yet for us) translate this to the corresponding versioned dep, and the error messages are also much nicer and earlier in case somebody tries to build with a too old version..

Re: [pbs-devel] [PATCH proxmox 1/2] sys: Use safe wrapper for libc::isatty

[Maximiliano Sandoval]

Setting the MSRV is always a good idea. Will post a v2.

Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:

>> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 19.11.2023 16:09 CET geschrieben:
>>
>> Am 17/11/2023 um 13:37 schrieb Maximiliano Sandoval R:
>> > Use the `std::io::IsTerminal` trait introduced in Rust 1.70.
>>
>> ack, but a more general question: should we set the msrv in the manifest:
>>
>> rust-version = "1.70"
>>
>> We did not bothered to much with that until now, only a bit indirectly through
>> increasing the edition to e.g., 2021, and we do not have that many external
>> users for it to matter much.
>
> I think that would make sense. most recent debcargo versions (not yet for us) translate this to the corresponding versioned dep, and the error messages are also much nicer and earlier in case somebody tries to build with a too old version..


--
Maximiliano