* Re: [pve-devel] [PATCH xtermjs] termproxy: rewrite in rust
[not found] <20200706105721.9241-1-d.csapak@proxmox.com>
@ 2020-07-07 9:34 ` Wolfgang Bumiller
0 siblings, 0 replies; only message in thread
From: Wolfgang Bumiller @ 2020-07-07 9:34 UTC (permalink / raw)
To: Dominik Csapak; +Cc: pve-devel
Some comments.
On Mon, Jul 06, 2020 at 12:57:21PM +0200, Dominik Csapak wrote:
> termproxy is now completely written in rust (instead of perl) but
> it is a drop-in replacement
(...)
> diff --git a/src/main.rs b/src/main.rs
> new file mode 100644
> index 0000000..459a3c0
> --- /dev/null
> +++ b/src/main.rs
> @@ -0,0 +1,378 @@
(...)
> +use std::io::{Error, ErrorKind, Result, Write};
None of our code currently imports a fixed `Result` type, please prefer
just typing `io::Result` (it's not that much longer).
(...)
> +
> +fn remove_number(buf: &mut ByteBuffer) -> Option<usize> {
> + loop {
> + let data = buf.get_data_slice();
> +
> + if let Some(pos) = data.iter().position(|&x| x == ':' as u8) {
use b':' (see `cargo +nightly clippy`)
> + let len = buf.consume(pos);
please don't use the name `len` for a `Box<[u8]>`, that's confusing
> + buf.consume(1); // the ':'
reminder: this `consume` currently returns a heap-allocated copy of the
consumed data (this should be changed in the ByteBuffer API)
> + let len = match String::from_utf8_lossy(&len).parse() {
`from_utf8_lossy` potentially allocates, but whenever it does, `parse()`
will always fail, so consider using `std::str::from_utf8` instead...
> + Ok(len) => len,
> + Err(err) => {
> + eprintln!("error parsing number: '{}'", err);
> + break;
> + }
> + };
> + return Some(len);
> + } else if data.len() > 20 {
> + buf.consume(20);
> + } else {
> + break;
> + }
> + }
> + None
> +}
> +
> +fn process_queue(buf: &mut ByteBuffer, pty: &mut PTY) -> Option<usize> {
> + if buf.is_empty() {
> + return None;
> + }
> +
> + loop {
> + let data = buf.get_data_slice();
> + let len = buf.data_size();
`len` is used only once here so maybe just drop it
> +
> + if len < 2 {
and then `data.len()` will be shorter and easier to follow than `buf.data_size()`
> + break;
> + }
> +
> + let msgtype = data[0] - '0' as u8;
b'0' again
> +
> + if msgtype == 0 {
please introduce some `const`s with meaningful names for message types
> + buf.consume(2);
> + if let Some(len) = remove_number(buf) {
> + return Some(len);
> + }
> + } else if msgtype == 1 {
> + buf.consume(2);
> + if let Some(cols) = remove_number(buf) {
> + if let Some(rows) = remove_number(buf) {
> + pty.set_size(cols as u16, rows as u16).ok()?;
> + }
Shouldn't the inner `if` have an `else`? `cols` was parsed successfully
here, so if there's no `rows` we'll re-loop back to grabbing the message
type, simply discarding the `cols` value?
> + }
> + // ignore incomplete messages
> + } else if msgtype == 2 {
> + buf.consume(1);
> + // ignore ping
> + } else {
> + buf.consume(1);
> + // ignore invalid
> + }
> + }
> +
> + None
> +}
> +
> +/// Reads from the stream and returns the first line and the rest
> +fn read_ticket_line(
> + stream: &mut TcpStream,
> + buf: &mut ByteBuffer,
> + timeout: u64,
`u64` is a really bad type for a timeout, it doesn't say anything about
the unit, also, the implementation isn't necessarily using time to
measure this (see below).
> +) -> Result<(Box<[u8]>, Box<[u8]>)> {
Clippy complains, but won't complaina bout a pair of `Vec<u8>` which you
already have anyway (but not really, see below...)
> + stream.set_read_timeout(Some(Duration::new(1, 0)))?;
This makes the reader believe that `timeout` will be in seconds but ...
> + let mut count = 0;
> + while !buf.get_data_slice().contains(&('\n' as u8)) {
> + if buf.is_full() || count >= timeout {
> + return Err(Error::new(
> + ErrorKind::InvalidData,
> + "authentication data is invalid",
> + ));
> + }
> + buf.read_from(stream)?;
> + count += 1;
... consider a short burst of 1-byte packets arriving, as many as the
number in `timeout`, then we also reach the count.
> + }
> +
> + stream.set_read_timeout(None)?;
> + let newline_idx = buf
> + .get_data_slice()
> + .iter()
> + .position(|&x| x == '\n' as u8)
> + .unwrap();
> +
> + let line = buf.consume(newline_idx);
> + buf.consume(1); // discard newline
> +
> + let mut iter = line.splitn(2, |&c| c == ':' as u8);
> + let username = iter.next().unwrap();
> + let ticket = iter.next().unwrap();
These `unwrap()`s are invalid. Consider using `.position()` to find the
b':' to have only 1 error case, then use the slice's `split_at()` method
to get a &[u8] 2-tuple:
match line.iter().position(|&b| b == b':') {
Some(pos) => {
let (username, ticket) = line.split_at(pos);
Ok((username.into(), ticket.into()))
}
None => bail!("with a nice error message"),
}
> + Ok((
> + username.to_vec().into_boxed_slice(),
> + ticket.to_vec().into_boxed_slice(),
> + ))
> +}
(...)
> +fn listen_and_accept(hostname: &str, port: u16, timeout: u64) -> Result<(TcpStream, SocketAddr)> {
> + let listener = TcpListener::bind((hostname, port))?;
> + listener.set_nonblocking(true)?;
This turns the loop below into a busy-wait loop. Please use `poll` on
the listen socket for the timeout.
> +
> + let now = Instant::now();
> + loop {
> + if now.elapsed().as_secs() > timeout {
> + return Err(Error::new(
> + ErrorKind::TimedOut,
> + "timed out waiting for client",
> + ));
> + }
> +
> + match listener.accept() {
> + Err(err) => {
> + if err.kind() == ErrorKind::WouldBlock {
> + continue;
> + } else {
> + return Err(err);
> + }
> + }
> + Ok(other) => return Ok(other),
> + };
> + }
> +}
> +
> +fn run_pty(cmd: &OsStr, params: clap::OsValues) -> Result<PTY> {
> + let (mut pty, secondary_name) = PTY::new().map_err(io_err_other)?;
> +
> + let mut filtered_env: HashMap<OsString, OsString> = std::env::vars_os()
> + .filter(|&(ref k, _)| {
> + k == "PATH" || k == "USER" || k == "HOME" || k == "LANG" || k == "LANGUAGE" ||
> + k.to_string_lossy().starts_with("LC_")
Please use `OsStrExt`'s .as_bytes() instead of allocating a new `String`
here.
> + }).collect();
> + filtered_env.insert("TERM".into(), "xterm-256color".into());
> +
> + let mut command = Command::new(cmd);
> +
> + command
> + .args(params)
> + .env_clear()
> + .envs(&filtered_env);
> +
> + unsafe {
> + command.pre_exec(move || {
> + make_controlling_terminal(&secondary_name).map_err(io_err_other)?;
> + Ok(())
> + });
> + }
> +
> + command.spawn()?;
> +
> + pty.set_size(80, 20).map_err(|x| x.as_errno().unwrap())?;
Another `unwrap` I don't find very convincing.
Consider using `into_io_error` from `proxmox::sys::error::SysError` or
`SysResult`.
> + Ok(pty)
> +}
> +
> +const TCP: Token = Token(0);
> +const PTY: Token = Token(1);
> +
> +fn main() -> Result<()> {
> + let matches = App::new("termproxy")
> + .setting(AppSettings::TrailingVarArg)
> + .arg(Arg::with_name("port").takes_value(true).required(true))
> + .arg(
> + Arg::with_name("authport")
> + .takes_value(true)
> + .long("authport"),
> + )
> + .arg(
> + Arg::with_name("path")
> + .takes_value(true)
> + .long("path")
> + .required(true),
> + )
> + .arg(Arg::with_name("perm").takes_value(true).long("perm"))
> + .arg(Arg::with_name("cmd").multiple(true).required(true))
> + .get_matches();
> +
> + let port: u16 = matches .value_of("port") .unwrap() .parse() .map_err(io_err_other)?;
> + let path = matches.value_of("path").unwrap();
> + let perm: Option<&str> = matches.value_of("perm");
> + let mut cmdparams = matches.values_of_os("cmd").unwrap();
> + let cmd = cmdparams.next().unwrap();
> + let authport: u16 = matches .value_of("authport") .unwrap_or("85") .parse() .map_err(io_err_other)?;
> + let mut pty_buf = ByteBuffer::new();
> + let mut tcp_buf = ByteBuffer::new();
> +
> + let (mut stream, client) = listen_and_accept("localhost", port, 10)?;
> +
> + let (username, ticket) = read_ticket_line(&mut stream, &mut pty_buf, 30)?;
> + authenticate(&username, &ticket, path, perm, authport)?;
> + stream.write_all("OK".as_bytes())?;
Use b"OK".
(...)
> + while tcp_readable && !pty_buf.is_full() {
> + let bytes = match pty_buf.read_from(&mut tcp_handle) {
You can handle `0` here:
Ok(0) => return Ok(()),
Skip the `let bytes =` binding and the `if bytes == 0` below.
Makes it a bit shorter.
> + Ok(bytes) => bytes,
> + Err(err) if err.kind() == ErrorKind::WouldBlock => {
> + tcp_readable = false;
> + break;
> + }
> + Err(err) => return Err(err),
> + };
> + if bytes == 0 {
> + return Ok(());
> + }
> + }
> +
> + while pty_readable && !tcp_buf.is_full() {
> + let bytes = match tcp_buf.read_from(&mut pty) {
> + Ok(bytes) => bytes,
> + Err(err) if err.kind() == ErrorKind::WouldBlock => {
> + pty_readable = false;
> + break;
> + }
> + Err(err) => return Err(err),
> + };
> + if bytes == 0 {
> + return Ok(());
> + }
> + }
> +
> + while !tcp_buf.is_empty() && tcp_writable {
> + let bytes = match tcp_handle.write(tcp_buf.get_data_slice()) {
> + Ok(bytes) => bytes,
Similarly the let binding can go and the consume call put in the Ok case
here.
It's a bit more concise and this part here is quite long repetitive
code...
> + Err(err) if err.kind() == ErrorKind::WouldBlock => {
> + tcp_writable = false;
> + break;
> + }
> + Err(err) => return Err(err),
> + };
> + tcp_buf.consume(bytes);
> + }
> +
> + while !pty_buf.is_empty() && pty_writable {
> + if remaining == 0 {
> + remaining = match process_queue(&mut pty_buf, &mut pty) {
> + Some(val) => val,
> + None => break,
> + };
> + }
> + let len = min(remaining, pty_buf.data_size());
> + let bytes = match pty.write(&pty_buf.get_data_slice()[0..len]) {
> + Ok(bytes) => bytes,
> + Err(err) if err.kind() == ErrorKind::WouldBlock => {
> + pty_writable = false;
> + break;
> + }
> + Err(err) => return Err(err),
> + };
> + remaining -= bytes;
> + pty_buf.consume(bytes);
> + }
> + }
> +}
^ permalink raw reply [flat|nested] only message in thread