Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

niri_ipc::NiriSocket; niri msg version; version checking on IPC #278

Merged
merged 3 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ anyhow = "1.0.81"
bitflags = "2.5.0"
clap = { version = "~4.4.18", features = ["derive"] }
serde = { version = "1.0.197", features = ["derive"] }
serde_json = "1.0.115"
tracing = { version = "0.1.40", features = ["max_level_trace", "release_max_level_debug"] }
tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
tracy-client = { version = "0.17.0", default-features = false }
Expand Down Expand Up @@ -67,7 +68,7 @@ portable-atomic = { version = "1.6.0", default-features = false, features = ["fl
profiling = "1.0.15"
sd-notify = "0.4.1"
serde.workspace = true
serde_json = "1.0.115"
serde_json.workspace = true
smithay-drm-extras.workspace = true
tracing-subscriber.workspace = true
tracing.workspace = true
Expand Down
1 change: 1 addition & 0 deletions niri-ipc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ repository.workspace = true
[dependencies]
clap = { workspace = true, optional = true }
serde.workspace = true
serde_json.workspace = true

[features]
clap = ["dep:clap"]
10 changes: 8 additions & 2 deletions niri-ipc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@ use std::str::FromStr;

use serde::{Deserialize, Serialize};

/// Name of the environment variable containing the niri IPC socket path.
pub const SOCKET_PATH_ENV: &str = "NIRI_SOCKET";
mod socket;
pub use socket::{Socket, SOCKET_PATH_ENV};

/// Request from client to niri.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub enum Request {
/// Request the version string for the running niri instance.
Version,
/// Request information about connected outputs.
Outputs,
/// Request information about the focused window.
FocusedWindow,
/// Perform an action.
Action(Action),
/// Respond with an error (for testing error handling).
ReturnError,
}

/// Reply from niri to client.
Expand All @@ -35,6 +39,8 @@ pub type Reply = Result<Response, String>;
pub enum Response {
/// A request that does not need a response was handled successfully.
Handled,
/// The version string for the running niri instance.
Version(String),
/// Information about connected outputs.
///
/// Map from connector name to output info.
Expand Down
63 changes: 63 additions & 0 deletions niri-ipc/src/socket.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
//! Helper for blocking communication over the niri socket.

use std::env;
use std::io::{self, Read, Write};
use std::net::Shutdown;
use std::os::unix::net::UnixStream;
use std::path::Path;

use crate::{Reply, Request};

/// Name of the environment variable containing the niri IPC socket path.
pub const SOCKET_PATH_ENV: &str = "NIRI_SOCKET";

/// Helper for blocking communication over the niri socket.
///
/// This struct is used to communicate with the niri IPC server. It handles the socket connection
/// and serialization/deserialization of messages.
pub struct Socket {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't like this generic name as much. When used in a file, it's easier to confuse with more general I/O operations when skimming (compare to naming of UnixStream, TcpStream, UdpSocket in the std library). It's an unimportant point though; this was an exhaustive list of all the downsides that such a name brings.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but I decided to do this because in other niri code I'm already using fully-qualified names for some types like niri_config::Animation and niri_ipc::Output (leaving non-qualified names for more frequently occurring types of the same name). So it's consistent this way.

stream: UnixStream,
}

impl Socket {
/// Connects to the default niri IPC socket.
///
/// This is equivalent to calling [`Self::connect_to`] with the path taken from the
/// [`SOCKET_PATH_ENV`] environment variable.
pub fn connect() -> io::Result<Self> {
let socket_path = env::var_os(SOCKET_PATH_ENV).ok_or_else(|| {
io::Error::new(
io::ErrorKind::NotFound,
format!("{SOCKET_PATH_ENV} is not set, are you running this within niri?"),
)
})?;
Self::connect_to(socket_path)
}

/// Connects to the niri IPC socket at the given path.
pub fn connect_to(path: impl AsRef<Path>) -> io::Result<Self> {
let stream = UnixStream::connect(path.as_ref())?;
Ok(Self { stream })
}

/// Sends a request to niri and returns the response.
///
/// Return values:
///
/// * `Ok(Ok(response))`: successful [`Response`](crate::Response) from niri
/// * `Ok(Err(message))`: error message from niri
/// * `Err(error)`: error communicating with niri
pub fn send(self, request: Request) -> io::Result<Reply> {
let Self { mut stream } = self;

let mut buf = serde_json::to_vec(&request).unwrap();
stream.write_all(&buf)?;
stream.shutdown(Shutdown::Write)?;

buf.clear();
stream.read_to_end(&mut buf)?;

let reply = serde_json::from_slice(&buf)?;
Ok(reply)
}
}
4 changes: 4 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ pub enum Sub {

#[derive(Subcommand)]
pub enum Msg {
/// Print the version of the running niri instance.
Version,
/// List connected outputs.
Outputs,
/// Print information about the focused window.
Expand All @@ -61,4 +63,6 @@ pub enum Msg {
#[command(subcommand)]
action: Action,
},
/// Request an error from the running niri instance.
RequestError,
}
127 changes: 84 additions & 43 deletions src/ipc/client.rs
Original file line number Diff line number Diff line change
@@ -1,54 +1,99 @@
use std::env;
use std::io::{Read, Write};
use std::net::Shutdown;
use std::os::unix::net::UnixStream;

use anyhow::{anyhow, bail, Context};
use niri_ipc::{LogicalOutput, Mode, Output, Reply, Request, Response};
use niri_ipc::{LogicalOutput, Mode, Output, Request, Response, Socket, Transform};
use serde_json::json;

use crate::cli::Msg;
use crate::utils::version;

pub fn handle_msg(msg: Msg, json: bool) -> anyhow::Result<()> {
let socket_path = env::var_os(niri_ipc::SOCKET_PATH_ENV).with_context(|| {
format!(
"{} is not set, are you running this within niri?",
niri_ipc::SOCKET_PATH_ENV
)
})?;

let mut stream =
UnixStream::connect(socket_path).context("error connecting to {socket_path}")?;

let request = match &msg {
Msg::Version => Request::Version,
Msg::Outputs => Request::Outputs,
Msg::FocusedWindow => Request::FocusedWindow,
Msg::Action { action } => Request::Action(action.clone()),
Msg::RequestError => Request::ReturnError,
};
let mut buf = serde_json::to_vec(&request).unwrap();
stream
.write_all(&buf)
.context("error writing IPC request")?;
stream
.shutdown(Shutdown::Write)
.context("error closing IPC stream for writing")?;

buf.clear();
stream
.read_to_end(&mut buf)
.context("error reading IPC response")?;
let socket = Socket::connect().context("error connecting to the niri socket")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: naming of niri_ipc::Socket (what? is there a better mechanism to add suggestions as replies to existing review comments?)

in that case, given that we use qualified niri_ipc::Window (among others), it should be more consistent across all niri code.

Suggested change
let socket = Socket::connect().context("error connecting to the niri socket")?;
let socket = niri_ipc::Socket::connect().context("error connecting to the niri socket")?;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file specifically, since it's under src/ipc/ and doesn't import any of the conflicting types, I am importing and using all niri_ipc types directly. The only exception is Transform which seems like an oversight; I'll also import it. (In src/ipc/server.rs it's clearer to fully qualify, because it's filling info from a Smithay Window).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although in general, for other types, i agree with that justification; Qualify them to indicate it's the niri_ipc version of that type when not in the context of IPC. But, see point about preferring NiriSocket over Socket (and i stand by that as it does avoid the need to qualify the name at all): In this context, while it's obvious that we're working with niri_ipc, it's not obvious what Socket means without viewing the imports or having seen the definition. Qualifying the name here is to disambiguate (for the reader) the concept of a "niri IPC socket" from "a generic type of socket with arbitrary data flowing in each direction" (e.g. they are different kinds of objects in the semantic sense); not to disambiguate the specific variant of "a transform but with serde attributes" from "a transform but the one Smithay likes" (i.e. they are the same thing semantically but distinct only in type systems and specific trait impls that both could just as well do)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can always change it later if we need


let reply: Reply = serde_json::from_slice(&buf).context("error parsing IPC reply")?;
let reply = socket
.send(request)
.context("error communicating with niri")?;

let response = reply
.map_err(|msg| anyhow!(msg))
.context("niri could not handle the request")?;
let compositor_version = match reply {
Err(_) if !matches!(msg, Msg::Version) => {
// If we got an error, it might be that the CLI is a different version from the running
// niri instance. Request the running instance version to compare and print a message.
Socket::connect()
.and_then(|socket| socket.send(Request::Version))
.ok()
}
_ => None,
};

// Default SIGPIPE so that our prints don't panic on stdout closing.
unsafe {
libc::signal(libc::SIGPIPE, libc::SIG_DFL);
}

let response = reply.map_err(|err_msg| {
// Check for CLI-server version mismatch to add helpful context.
match compositor_version {
Some(Ok(Response::Version(compositor_version))) => {
let cli_version = version();
if cli_version != compositor_version {
eprintln!("Running niri compositor has a different version from the niri CLI:");
eprintln!("Compositor version: {compositor_version}");
eprintln!("CLI version: {cli_version}");
eprintln!("Did you forget to restart niri after an update?");
Comment on lines +45 to +48
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer your phrasing to my original version. "Running niri compositor" is better than "the compositor".

eprintln!();
}
}
Some(_) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block now handles the Reply::Ok case for non-Response::Version. Is that desirable? This wouldn't be caused by an outdated compositor (it would cause a Reply::Err("error parsing request")), and as such i made it not print any kind of error response. In reality, it should be unreachable since the compositor server Does Not Behave Like This

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is intentional for the reason that you mentioned. If it ever happens then something weird is going on; doesn't hurt to show the warning.

eprintln!("Unable to get the running niri compositor version.");
eprintln!("Did you forget to restart niri after an update?");
eprintln!();
}
None => {
// Communication error, or the original request was already a version request.
// Don't add irrelevant context.
}
}

anyhow!(err_msg).context("niri returned an error")
})?;

match msg {
Msg::RequestError => {
bail!("unexpected response: expected an error, got {response:?}");
}
Msg::Version => {
let Response::Version(compositor_version) = response else {
bail!("unexpected response: expected Version, got {response:?}");
};

let cli_version = version();

if json {
println!(
"{}",
json!({
"compositor": compositor_version,
"cli": cli_version,
})
);
return Ok(());
}

if cli_version != compositor_version {
eprintln!("Running niri compositor has a different version from the niri CLI.");
eprintln!("Did you forget to restart niri after an update?");
eprintln!();
}

println!("Compositor version: {compositor_version}");
println!("CLI version: {cli_version}");
}
Msg::Outputs => {
let Response::Outputs(outputs) = response else {
bail!("unexpected response: expected Outputs, got {response:?}");
Expand Down Expand Up @@ -123,18 +168,14 @@ pub fn handle_msg(msg: Msg, json: bool) -> anyhow::Result<()> {
println!(" Scale: {scale}");

let transform = match transform {
niri_ipc::Transform::Normal => "normal",
niri_ipc::Transform::_90 => "90° counter-clockwise",
niri_ipc::Transform::_180 => "180°",
niri_ipc::Transform::_270 => "270° counter-clockwise",
niri_ipc::Transform::Flipped => "flipped horizontally",
niri_ipc::Transform::Flipped90 => {
"90° counter-clockwise, flipped horizontally"
}
niri_ipc::Transform::Flipped180 => "flipped vertically",
niri_ipc::Transform::Flipped270 => {
"270° counter-clockwise, flipped horizontally"
}
Transform::Normal => "normal",
Transform::_90 => "90° counter-clockwise",
Transform::_180 => "180°",
Transform::_270 => "270° counter-clockwise",
Transform::Flipped => "flipped horizontally",
Transform::Flipped90 => "90° counter-clockwise, flipped horizontally",
Transform::Flipped180 => "flipped vertically",
Transform::Flipped270 => "270° counter-clockwise, flipped horizontally",
};
println!(" Transform: {transform}");
}
Expand Down
25 changes: 17 additions & 8 deletions src/ipc/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use calloop::io::Async;
use directories::BaseDirs;
use futures_util::io::{AsyncReadExt, BufReader};
use futures_util::{AsyncBufReadExt, AsyncWriteExt};
use niri_ipc::{Request, Response};
use niri_ipc::{Reply, Request, Response};
use smithay::desktop::Window;
use smithay::reexports::calloop::generic::Generic;
use smithay::reexports::calloop::{Interest, LoopHandle, Mode, PostAction};
Expand All @@ -18,6 +18,7 @@ use smithay::wayland::shell::xdg::XdgToplevelSurfaceData;

use crate::backend::IpcOutputMap;
use crate::niri::State;
use crate::utils::version;

pub struct IpcServer {
pub socket_path: PathBuf,
Expand Down Expand Up @@ -114,21 +115,29 @@ async fn handle_client(ctx: ClientCtx, stream: Async<'_, UnixStream>) -> anyhow:
.await
.context("error reading request")?;

let reply = process(&ctx, &buf).map_err(|err| {
warn!("error processing IPC request: {err:?}");
err.to_string()
});
let request = serde_json::from_str(&buf)
.context("error parsing request")
.map_err(|err| err.to_string());
let requested_error = matches!(request, Ok(Request::ReturnError));

let reply = request.and_then(|request| process(&ctx, request));

if let Err(err) = &reply {
if !requested_error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to handle this error flow differently? It's not really problematic to print it; nobody's gonna spam niri msg request-error and then complain that the log output contains the same message; conversely, it's reasonable that someone may want to see what it looks like on the compositor side handling an error in which case this check somewhat obscures it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no hard opinions here, but in my head this request is aimed much more at the client side, so as far as the server is concerned it shouldn't print a warning.

warn!("error processing IPC request: {err:?}");
}
}

let buf = serde_json::to_vec(&reply).context("error formatting reply")?;
write.write_all(&buf).await.context("error writing reply")?;

Ok(())
}

fn process(ctx: &ClientCtx, buf: &str) -> anyhow::Result<Response> {
let request: Request = serde_json::from_str(buf).context("error parsing request")?;

fn process(ctx: &ClientCtx, request: Request) -> Reply {
let response = match request {
Request::ReturnError => return Err(String::from("example compositor error")),
Request::Version => Response::Version(version()),
Request::Outputs => {
let ipc_outputs = ctx.ipc_outputs.lock().unwrap().clone();
Response::Outputs(ipc_outputs)
Expand Down