[pve-devel] [PATCH 1/2] use anyhow for result/error

[pve-devel] [PATCH 1/2] use anyhow for result/error

[Thomas Lamprecht]

it's in out dependency chain anyway through proxmox-sys or -time and
it makes life a bit easier. FWIW, I got slightly pressured into this
by the future switch from curl to ureq, as the latter brings their
own Error type.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

Not a must, we can handle the ureq::Error otherwise, but as we already
depend on anyhow indirectly I figured to go for it...

 Cargo.toml  |  1 +
 src/main.rs | 34 +++++++++++++++++-----------------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 60c3ae0..25c4569 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,6 +11,7 @@ license = "AGPL-3"
 exclude = [ "build", "debian" ]
 
 [dependencies]
+anyhow = "1"
 mio = { version = "0.7", features = [ "net", "os-ext" ] }
 curl = "0.4"
 clap = "2.33"
diff --git a/src/main.rs b/src/main.rs
index 7896a9c..0003834 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,12 +1,13 @@
 use std::cmp::min;
 use std::collections::HashMap;
 use std::ffi::{OsStr, OsString};
-use std::io::{ErrorKind, Result, Write};
+use std::io::{ErrorKind, Write};
 use std::os::unix::io::{AsRawFd, FromRawFd};
 use std::os::unix::process::CommandExt;
 use std::process::Command;
 use std::time::{Duration, Instant};
 
+use anyhow::{bail, format_err, Result};
 use clap::{App, AppSettings, Arg};
 use curl::easy::Easy;
 use mio::net::{TcpListener, TcpStream};
@@ -16,7 +17,6 @@ use mio::{Events, Interest, Poll, Token};
 use proxmox_io::ByteBuffer;
 use proxmox_sys::{
     error::io_err_other,
-    io_bail, io_format_err,
     linux::pty::{make_controlling_terminal, PTY},
 };
 
@@ -108,11 +108,11 @@ fn read_ticket_line(
             match buf.read_from(stream) {
                 Ok(n) => {
                     if n == 0 {
-                        io_bail!("connection closed before authentication");
+                        bail!("connection closed before authentication");
                     }
                 }
                 Err(err) if err.kind() == ErrorKind::WouldBlock => {}
-                Err(err) => return Err(err),
+                Err(err) => return Err(err.into()),
             }
 
             if buf[..].contains(&b'\n') {
@@ -120,13 +120,13 @@ fn read_ticket_line(
             }
 
             if buf.is_full() {
-                io_bail!("authentication data is incomplete: {:?}", &buf[..]);
+                bail!("authentication data is incomplete: {:?}", &buf[..]);
             }
         }
 
         elapsed = now.elapsed();
         if elapsed > timeout {
-            io_bail!("timed out");
+            bail!("timed out");
         }
     }
 
@@ -140,7 +140,7 @@ fn read_ticket_line(
             let (username, ticket) = line.split_at(pos);
             Ok((username.into(), ticket[1..].into()))
         }
-        None => io_bail!("authentication data is invalid"),
+        None => bail!("authentication data is invalid"),
     }
 }
 
@@ -183,7 +183,7 @@ fn authenticate(
     let response_code = curl.response_code()?;
 
     if response_code != 200 {
-        io_bail!("invalid authentication, code {}", response_code);
+        bail!("invalid authentication, code {}", response_code);
     }
 
     Ok(())
@@ -222,7 +222,7 @@ fn listen_and_accept(
 
         elapsed = now.elapsed();
         if elapsed > timeout {
-            io_bail!("timed out");
+            bail!("timed out");
         }
     }
 }
@@ -302,17 +302,17 @@ fn do_main() -> Result<()> {
     let use_port_as_fd = matches.is_present("use-port-as-fd");
 
     if use_port_as_fd && port > u16::MAX as u64 {
-        return Err(io_format_err!("port too big"));
+        return Err(format_err!("port too big"));
     } else if port > i32::MAX as u64 {
-        return Err(io_format_err!("Invalid FD number"));
+        return Err(format_err!("Invalid FD number"));
     }
 
     let (mut tcp_handle, port) =
         listen_and_accept("localhost", port, use_port_as_fd, Duration::new(10, 0))
-            .map_err(|err| io_format_err!("failed waiting for client: {}", err))?;
+            .map_err(|err| format_err!("failed waiting for client: {}", err))?;
 
     let (username, ticket) = read_ticket_line(&mut tcp_handle, &mut pty_buf, Duration::new(10, 0))
-        .map_err(|err| io_format_err!("failed reading ticket: {}", err))?;
+        .map_err(|err| format_err!("failed reading ticket: {}", err))?;
     let port = if use_port_as_fd { Some(port) } else { None };
     authenticate(&username, &ticket, path, perm, authport, port)?;
     tcp_handle.write_all(b"OK").expect("error writing response");
@@ -383,7 +383,7 @@ fn do_main() -> Result<()> {
                 }
                 Err(err) => {
                     if !finished {
-                        return Err(io_format_err!("error reading from tcp: {}", err));
+                        return Err(format_err!("error reading from tcp: {}", err));
                     }
                     break;
                 }
@@ -403,7 +403,7 @@ fn do_main() -> Result<()> {
                 }
                 Err(err) => {
                     if !finished {
-                        return Err(io_format_err!("error reading from pty: {}", err));
+                        return Err(format_err!("error reading from pty: {}", err));
                     }
                     break;
                 }
@@ -423,7 +423,7 @@ fn do_main() -> Result<()> {
                 }
                 Err(err) => {
                     if !finished {
-                        return Err(io_format_err!("error writing to tcp : {}", err));
+                        return Err(format_err!("error writing to tcp : {}", err));
                     }
                     break;
                 }
@@ -447,7 +447,7 @@ fn do_main() -> Result<()> {
                 }
                 Err(err) => {
                     if !finished {
-                        return Err(io_format_err!("error writing to pty : {}", err));
+                        return Err(format_err!("error writing to pty : {}", err));
                     }
                     break;
                 }
-- 
2.34.1

[pve-devel] [PATCH 2/2] switch from curl to ureq

[Thomas Lamprecht]

Not only less code, we lose a whopping 29 libraries on linkage cruft:

--- ldd.before  2022-01-26 19:03:25.759426164 +0100
+++ ldd.after   2022-01-26 19:03:31.919529632 +0100
@@ -5,36 +5,7 @@ librt.so.1
 libzstd.so.1
 libc.so.6
 /lib64/ld-linux-x86-64.so.2
-libcurl-gnutls.so.4
 libgcc_s.so.1
 libpthread.so.0
 libm.so.6
 libdl.so.2
-libnghttp2.so.14
-libidn2.so.0
-librtmp.so.1
-libssh2.so.1
-libpsl.so.5
-libnettle.so.8
-libgnutls.so.30
-libgssapi_krb5.so.2
-libldap_r-2.4.so.2
-liblber-2.4.so.2
-libbrotlidec.so.1
-libz.so.1
-libunistring.so.2
-libhogweed.so.6
-libgmp.so.10
-libgcrypt.so.20
-libp11-kit.so.0
-libtasn1.so.6
-libkrb5.so.3
-libk5crypto.so.3
-libcom_err.so.2
-libkrb5support.so.0
-libresolv.so.2
-libsasl2.so.2
-libbrotlicommon.so.1
-libgpg-error.so.0
-libffi.so.7
-libkeyutils.so.1

IOW.: curl is really nice for a CLI tool, but way to much overkill for simple
HTTP requests.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

IMO a no brainer, but I only did a quick "does it still work in general"
testing, I mean that *is* covering already quite a bit here ;-)

 Cargo.toml  |  2 +-
 src/main.rs | 41 ++++++++++++++---------------------------
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 25c4569..7f55edc 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -13,7 +13,7 @@ exclude = [ "build", "debian" ]
 [dependencies]
 anyhow = "1"
 mio = { version = "0.7", features = [ "net", "os-ext" ] }
-curl = "0.4"
+ureq = { version = "2.4", default-features = false, features = [ "gzip", ] }
 clap = "2.33"
 proxmox-io = "1"
 proxmox-sys = "0.2"
diff --git a/src/main.rs b/src/main.rs
index 0003834..29508e6 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -9,7 +9,6 @@ use std::time::{Duration, Instant};
 
 use anyhow::{bail, format_err, Result};
 use clap::{App, AppSettings, Arg};
-use curl::easy::Easy;
 use mio::net::{TcpListener, TcpStream};
 use mio::unix::SourceFd;
 use mio::{Events, Interest, Poll, Token};
@@ -152,38 +151,26 @@ fn authenticate(
     authport: u16,
     port: Option<u16>,
 ) -> Result<()> {
-    let mut curl = Easy::new();
-    curl.url(&format!(
-        "http://localhost:{}/api2/json/access/ticket",
-        authport
-    ))?;
-
-    let username = curl.url_encode(username);
-    let ticket = curl.url_encode(ticket);
-    let path = curl.url_encode(path.as_bytes());
-
-    let mut post_fields = Vec::with_capacity(5);
-    post_fields.push(format!("username={}", username));
-    post_fields.push(format!("password={}", ticket));
-    post_fields.push(format!("path={}", path));
-
+    let mut post_fields: Vec<(&str, &str)> = Vec::with_capacity(5);
+    post_fields.append(& mut vec![
+        ("username", std::str::from_utf8(username)?),
+        ("password", std::str::from_utf8(ticket)?),
+        ("path", path),
+    ]);
     if let Some(perm) = perm {
-        let perm = curl.url_encode(perm.as_bytes());
-        post_fields.push(format!("privs={}", perm));
+        post_fields.push(("privs", perm));
     }
-
+    let port_str;
     if let Some(port) = port {
-        post_fields.push(format!("port={}", port));
+        port_str = port.to_string();
+        post_fields.push(("port", &port_str));
     }
 
-    curl.post_fields_copy(post_fields.join("&").as_bytes())?;
-    curl.post(true)?;
-    curl.perform()?;
+    let url = format!("http://localhost:{}/api2/json/access/ticket", authport);
+    let res = ureq::post(&url).send_form(&post_fields[..])?;
 
-    let response_code = curl.response_code()?;
-
-    if response_code != 200 {
-        bail!("invalid authentication, code {}", response_code);
+    if res.status() != 200 {
+        bail!("invalid authentication, {} - {}", res.status(), res.status_text());
     }
 
     Ok(())
-- 
2.34.1

Re: [pve-devel] [PATCH 1/2] use anyhow for result/error

[Dominik Csapak]

looks sane and works (tested on pve & pbs)
did not push them myself since the ldd diff made it
necessary that i edited the patch file before applying locally...

so, for both patches:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

[pve-devel] applied-series: [PATCH 1/2] use anyhow for result/error

[Thomas Lamprecht]

On 27.01.22 13:39, Dominik Csapak wrote:
> looks sane and works (tested on pve & pbs)
> did not push them myself since the ldd diff made it
> necessary that i edited the patch file before applying locally...

oops, yeah makes sense - I now added '> ' quoting in front of it to avoid
that.

> so, for both patches:
> 
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>

Thanks! I added those for the first patch but I had to change 2/2 slightly,
as ureq returns an error-result for all responses > 400 and so I had to
match that to get it to print our error and strictly allow only 200 codes.