Skip to content

Commit

Permalink
Simplify server errors and drop some unused code (#1095)
Browse files Browse the repository at this point in the history 
* Simplify the error handling in the `agama-server` crate, unifying
many.
* Remove some functions from the `Locale` D-Bus API that are irrelevant
for inter-process communication.
* +88/-215 lines and removed some redundant `IntoResponse`
implementations.
  • Loading branch information
@imobachgs
imobachgs authored Mar 18, 2024
2 parents dc94f6c + 80ac668 commit 805e9bd
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 215 deletions.
35 changes: 29 additions & 6 deletions rust/agama-server/src/error.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
use zbus_macros::DBusError;
use agama_lib::error::ServiceError;
use axum::{
http::StatusCode,
response::{IntoResponse, Response},
Json,
};
use serde_json::json;

#[derive(DBusError, Debug)]
#[dbus_error(prefix = "org.opensuse.Agama1.Locale")]
use crate::{l10n::web::LocaleError, questions::QuestionsError};

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[dbus_error(zbus_error)]
ZBus(zbus::Error),
#[error("D-Bus error: {0}")]
DBus(#[from] zbus::Error),
#[error("Generic error: {0}")]
Anyhow(String),
#[error("Agama service error: {0}")]
Service(#[from] ServiceError),
#[error("Questions service error: {0}")]
Questions(QuestionsError),
#[error("Software service error: {0}")]
Locale(#[from] LocaleError),
}

// This would be nice, but using it for a return type
Expand All @@ -22,6 +36,15 @@ impl From<anyhow::Error> for Error {

impl From<Error> for zbus::fdo::Error {
fn from(value: Error) -> zbus::fdo::Error {
zbus::fdo::Error::Failed(format!("Localization error: {value}"))
zbus::fdo::Error::Failed(format!("D-Bus error: {value}"))
}
}

impl IntoResponse for Error {
fn into_response(self) -> Response {
let body = json!({
"error": self.to_string()
});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
}
83 changes: 12 additions & 71 deletions rust/agama-server/src/l10n.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub mod web;

use crate::error::Error;
use agama_locale_data::{KeymapId, LocaleId};
use anyhow::Context;

use keyboard::KeymapsDatabase;
use locale::LocalesDatabase;
use regex::Regex;
Expand All @@ -32,38 +32,18 @@ pub struct Locale {

#[dbus_interface(name = "org.opensuse.Agama1.Locale")]
impl Locale {
/// Gets the supported locales information.
///
/// Each element of the list has these parts:
///
/// * The locale code (e.g., "es_ES.UTF-8").
/// * The name of the language according to the language defined by the
/// UILocale property.
/// * The name of the territory according to the language defined by the
/// UILocale property.
fn list_locales(&self) -> Result<Vec<(String, String, String)>, Error> {
let locales = self
.locales_db
.entries()
.iter()
.map(|l| {
(
l.id.to_string(),
l.language.to_string(),
l.territory.to_string(),
)
})
.collect::<Vec<_>>();
Ok(locales)
}

#[dbus_interface(property)]
fn locales(&self) -> Vec<String> {
self.locales.to_owned()
}

#[dbus_interface(property)]
fn set_locales(&mut self, locales: Vec<String>) -> zbus::fdo::Result<()> {
if locales.is_empty() {
return Err(zbus::fdo::Error::Failed(format!(
"The locales list cannot be empty"
)));
}
for loc in &locales {
if !self.locales_db.exists(loc.as_str()) {
return Err(zbus::fdo::Error::Failed(format!(
Expand All @@ -89,22 +69,6 @@ impl Locale {
Ok(self.translate(&locale)?)
}

/// Returns a list of the supported keymaps.
///
/// Each element of the list contains:
///
/// * The keymap identifier (e.g., "es" or "es(ast)").
/// * The name of the keyboard in language set by the UILocale property.
fn list_keymaps(&self) -> Result<Vec<(String, String)>, Error> {
let keymaps = self
.keymaps_db
.entries()
.iter()
.map(|k| (k.id.to_string(), k.localized_description()))
.collect();
Ok(keymaps)
}

#[dbus_interface(property)]
fn keymap(&self) -> String {
self.keymap.to_string()
Expand All @@ -125,32 +89,6 @@ impl Locale {
Ok(())
}

/// Returns a list of the supported timezones.
///
/// Each element of the list contains:
///
/// * The timezone identifier (e.g., "Europe/Berlin").
/// * A list containing each part of the name in the language set by the
/// UILocale property.
/// * The name, in the language set by UILocale, of the main country
/// associated to the timezone (typically, the name of the city that is
/// part of the identifier) or empty string if there is no country.
fn list_timezones(&self) -> Result<Vec<(String, Vec<String>, String)>, Error> {
let timezones: Vec<_> = self
.timezones_db
.entries()
.iter()
.map(|tz| {
(
tz.code.to_string(),
tz.parts.clone(),
tz.country.clone().unwrap_or_default(),
)
})
.collect();
Ok(timezones)
}

#[dbus_interface(property)]
fn timezone(&self) -> &str {
self.timezone.as_str()
Expand All @@ -169,22 +107,25 @@ impl Locale {
}

// TODO: what should be returned value for commit?
fn commit(&mut self) -> Result<(), Error> {
fn commit(&mut self) -> zbus::fdo::Result<()> {
const ROOT: &str = "/mnt";

Command::new("/usr/bin/systemd-firstboot")
.args([
"--root",
ROOT,
"--force",
"--locale",
self.locales.first().context("missing locale")?.as_str(),
self.locales.first().unwrap_or(&"en_US.UTF-8".to_string()),
"--keymap",
&self.keymap.to_string(),
"--timezone",
&self.timezone,
])
.status()
.context("Failed to execute systemd-firstboot")?;
.map_err(|e| {
zbus::fdo::Error::Failed(format!("Could not apply the l10n configuration: {e}"))
})?;

Ok(())
}
Expand Down
38 changes: 13 additions & 25 deletions rust/agama-server/src/l10n/web.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,25 @@ use crate::{
use agama_locale_data::{InvalidKeymap, LocaleId};
use axum::{
extract::State,
http::StatusCode,
response::{IntoResponse, Response},
routing::{get, put},
Json, Router,
};
use serde::{Deserialize, Serialize};
use serde_json::json;
use std::{
process::Command,
sync::{Arc, RwLock},
};
use thiserror::Error;

#[derive(Error, Debug)]
#[derive(thiserror::Error, Debug)]
pub enum LocaleError {
#[error("Unknown locale code: {0}")]
UnknownLocale(String),
#[error("Unknown timezone: {0}")]
UnknownTimezone(String),
#[error("Invalid keymap: {0}")]
InvalidKeymap(#[from] InvalidKeymap),
#[error("Cannot translate: {0}")]
OtherError(#[from] Error),
#[error("Cannot change the local keymap: {0}")]
CouldNotSetKeymap(#[from] std::io::Error),
}

impl IntoResponse for LocaleError {
fn into_response(self) -> Response {
let body = json!({
"error": self.to_string()
});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
#[error("Could not apply the changes")]
Commit(#[from] std::io::Error),
}

#[derive(Clone)]
Expand Down Expand Up @@ -119,14 +104,14 @@ async fn keymaps(State(state): State<LocaleState>) -> Json<Vec<Keymap>> {
async fn set_config(
State(state): State<LocaleState>,
Json(value): Json<LocaleConfig>,
) -> Result<Json<()>, LocaleError> {
) -> Result<Json<()>, Error> {
let mut data = state.locale.write().unwrap();
let mut changes = LocaleConfig::default();

if let Some(locales) = &value.locales {
for loc in locales {
if !data.locales_db.exists(loc.as_str()) {
return Err(LocaleError::UnknownLocale(loc.to_string()));
return Err(LocaleError::UnknownLocale(loc.to_string()))?;
}
}
data.locales = locales.clone();
Expand All @@ -135,14 +120,14 @@ async fn set_config(

if let Some(timezone) = &value.timezone {
if !data.timezones_db.exists(timezone) {
return Err(LocaleError::UnknownTimezone(timezone.to_string()));
return Err(LocaleError::UnknownTimezone(timezone.to_string()))?;
}
data.timezone = timezone.to_owned();
changes.timezone = Some(data.timezone.clone());
}

if let Some(keymap_id) = &value.keymap {
data.keymap = keymap_id.parse()?;
data.keymap = keymap_id.parse().map_err(LocaleError::InvalidKeymap)?;
changes.keymap = Some(keymap_id.clone());
}

Expand All @@ -161,14 +146,17 @@ async fn set_config(
}

if let Some(ui_keymap) = &value.ui_keymap {
data.ui_keymap = ui_keymap.parse()?;
// data.ui_keymap = ui_keymap.parse().into::<Result<KeymapId, LocaleError>>()?;
data.ui_keymap = ui_keymap.parse().map_err(LocaleError::InvalidKeymap)?;
Command::new("/usr/bin/localectl")
.args(["set-x11-keymap", &ui_keymap])
.output()?;
.output()
.map_err(LocaleError::Commit)?;
Command::new("/usr/bin/setxkbmap")
.arg(&ui_keymap)
.env("DISPLAY", ":0")
.output()?;
.output()
.map_err(LocaleError::Commit)?;
}

_ = state.events.send(Event::L10nConfigChanged(changes));
Expand Down
25 changes: 4 additions & 21 deletions rust/agama-server/src/manager/web.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@ use agama_lib::{
};
use axum::{
extract::State,
http::StatusCode,
response::{IntoResponse, Response},
routing::{get, post},
Json, Router,
};
use serde::Serialize;
use serde_json::json;
use thiserror::Error;
use tokio_stream::{Stream, StreamExt};

use crate::{
Expand All @@ -37,19 +33,6 @@ pub struct ManagerState<'a> {
manager: ManagerClient<'a>,
}

#[derive(Error, Debug)]
pub enum ManagerError {
#[error("Manager service error: {0}")]
Error(#[from] ServiceError),
}

impl IntoResponse for ManagerError {
fn into_response(self) -> Response {
let body = json!({});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
}

/// Holds information about the manager's status.
#[derive(Clone, Serialize, utoipa::ToSchema)]
pub struct InstallerStatus {
Expand Down Expand Up @@ -115,7 +98,7 @@ pub async fn manager_service(dbus: zbus::Connection) -> Result<Router, ServiceEr
#[utoipa::path(get, path = "/api/manager/probe", responses(
(status = 200, description = "The probing process was started.")
))]
async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.probe().await?;
Ok(())
}
Expand All @@ -124,7 +107,7 @@ async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), Manag
#[utoipa::path(get, path = "/api/manager/install", responses(
(status = 200, description = "The installation process was started.")
))]
async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.install().await?;
Ok(())
}
Expand All @@ -133,7 +116,7 @@ async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), Man
#[utoipa::path(get, path = "/api/manager/install", responses(
(status = 200, description = "The installation tasks are executed.")
))]
async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.finish().await?;
Ok(())
}
Expand All @@ -144,7 +127,7 @@ async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), Mana
))]
async fn installer_status(
State(state): State<ManagerState<'_>>,
) -> Result<Json<InstallerStatus>, ManagerError> {
) -> Result<Json<InstallerStatus>, Error> {
let status = InstallerStatus {
phase: state.manager.current_installation_phase().await?,
busy: state.manager.busy_services().await?,
Expand Down
Loading

0 comments on commit 805e9bd

Please sign in to comment.