[pbs-devel] [PATCH proxmox 3/6] apt: avoid direct impl on api types (use traits instead)
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Jul 9 08:18:29 CEST 2024
From: Dietmar Maurer <dietmar at proxmox.com>
So that we can use api types from expternal crate proxmox-apt-api-types.
Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
---
proxmox-apt/src/repositories/file.rs | 68 ++++++++++++-------
.../src/repositories/file/list_parser.rs | 4 +-
.../src/repositories/file/sources_parser.rs | 1 +
proxmox-apt/src/repositories/mod.rs | 3 +
proxmox-apt/src/repositories/repository.rs | 60 ++++++++++------
proxmox-apt/src/repositories/standard.rs | 38 +++++++----
proxmox-apt/tests/repositories.rs | 3 +
7 files changed, 118 insertions(+), 59 deletions(-)
diff --git a/proxmox-apt/src/repositories/file.rs b/proxmox-apt/src/repositories/file.rs
index 0d21ce3c..086abf49 100644
--- a/proxmox-apt/src/repositories/file.rs
+++ b/proxmox-apt/src/repositories/file.rs
@@ -9,6 +9,8 @@ use crate::repositories::repository::{
APTRepository, APTRepositoryFileType, APTRepositoryPackageType,
};
+use crate::repositories::repository::APTRepositoryImpl;
+
use proxmox_schema::api;
mod list_parser;
@@ -116,13 +118,46 @@ pub struct APTRepositoryInfo {
pub message: String,
}
-impl APTRepositoryFile {
+pub trait APTRepositoryFileImpl {
/// Creates a new `APTRepositoryFile` without parsing.
///
/// If the file is hidden, the path points to a directory, or the extension
/// is usually ignored by APT (e.g. `.orig`), `Ok(None)` is returned, while
/// invalid file names yield an error.
- pub fn new<P: AsRef<Path>>(path: P) -> Result<Option<Self>, APTRepositoryFileError> {
+ fn new<P: AsRef<Path>>(path: P) -> Result<Option<APTRepositoryFile>, APTRepositoryFileError>;
+
+ fn with_content(content: String, content_type: APTRepositoryFileType) -> Self;
+
+ /// Check if the file exists.
+ fn exists(&self) -> bool;
+
+ fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError>;
+
+ /// Create an `APTRepositoryFileError`.
+ fn err(&self, error: Error) -> APTRepositoryFileError;
+
+ /// Parses the APT repositories configured in the file on disk, including
+ /// disabled ones.
+ ///
+ /// Resets the current repositories and digest, even on failure.
+ fn parse(&mut self) -> Result<(), APTRepositoryFileError>;
+
+ /// Writes the repositories to the file on disk.
+ ///
+ /// If a digest is provided, checks that the current content of the file still
+ /// produces the same one.
+ fn write(&self) -> Result<(), APTRepositoryFileError>;
+
+ /// Checks if old or unstable suites are configured and that the Debian security repository
+ /// has the correct suite. Also checks that the `stable` keyword is not used.
+ fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo>;
+
+ /// Checks for official URIs.
+ fn check_uris(&self) -> Vec<APTRepositoryInfo>;
+}
+
+impl APTRepositoryFileImpl for APTRepositoryFile {
+ fn new<P: AsRef<Path>>(path: P) -> Result<Option<Self>, APTRepositoryFileError> {
let path: PathBuf = path.as_ref().to_path_buf();
let new_err = |path_string: String, err: &str| APTRepositoryFileError {
@@ -197,7 +232,7 @@ impl APTRepositoryFile {
}))
}
- pub fn with_content(content: String, content_type: APTRepositoryFileType) -> Self {
+ fn with_content(content: String, content_type: APTRepositoryFileType) -> Self {
Self {
file_type: content_type,
content: Some(content),
@@ -207,8 +242,7 @@ impl APTRepositoryFile {
}
}
- /// Check if the file exists.
- pub fn exists(&self) -> bool {
+ fn exists(&self) -> bool {
if let Some(path) = &self.path {
PathBuf::from(path).exists()
} else {
@@ -216,7 +250,7 @@ impl APTRepositoryFile {
}
}
- pub fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError> {
+ fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError> {
if let Some(path) = &self.path {
let content = std::fs::read(path).map_err(|err| self.err(format_err!("{}", err)))?;
let digest = openssl::sha::sha256(&content);
@@ -233,19 +267,14 @@ impl APTRepositoryFile {
}
}
- /// Create an `APTRepositoryFileError`.
- pub fn err(&self, error: Error) -> APTRepositoryFileError {
+ fn err(&self, error: Error) -> APTRepositoryFileError {
APTRepositoryFileError {
path: self.path.clone().unwrap_or_default(),
error: error.to_string(),
}
}
- /// Parses the APT repositories configured in the file on disk, including
- /// disabled ones.
- ///
- /// Resets the current repositories and digest, even on failure.
- pub fn parse(&mut self) -> Result<(), APTRepositoryFileError> {
+ fn parse(&mut self) -> Result<(), APTRepositoryFileError> {
self.repositories.clear();
self.digest = None;
@@ -269,11 +298,7 @@ impl APTRepositoryFile {
Ok(())
}
- /// Writes the repositories to the file on disk.
- ///
- /// If a digest is provided, checks that the current content of the file still
- /// produces the same one.
- pub fn write(&self) -> Result<(), APTRepositoryFileError> {
+ fn write(&self) -> Result<(), APTRepositoryFileError> {
let path = match &self.path {
Some(path) => path,
None => {
@@ -336,9 +361,7 @@ impl APTRepositoryFile {
Ok(())
}
- /// Checks if old or unstable suites are configured and that the Debian security repository
- /// has the correct suite. Also checks that the `stable` keyword is not used.
- pub fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo> {
+ fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo> {
let mut infos = vec![];
let path = match &self.path {
@@ -427,8 +450,7 @@ impl APTRepositoryFile {
infos
}
- /// Checks for official URIs.
- pub fn check_uris(&self) -> Vec<APTRepositoryInfo> {
+ fn check_uris(&self) -> Vec<APTRepositoryInfo> {
let mut infos = vec![];
let path = match &self.path {
diff --git a/proxmox-apt/src/repositories/file/list_parser.rs b/proxmox-apt/src/repositories/file/list_parser.rs
index 0edeea7f..93bbcc12 100644
--- a/proxmox-apt/src/repositories/file/list_parser.rs
+++ b/proxmox-apt/src/repositories/file/list_parser.rs
@@ -3,9 +3,9 @@ use std::iter::Iterator;
use anyhow::{bail, format_err, Error};
-use crate::repositories::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
-
use super::APTRepositoryParser;
+use crate::repositories::APTRepositoryImpl;
+use crate::repositories::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
// TODO convert %-escape characters. Also adapt printing back accordingly,
// because at least '%' needs to be re-escaped when printing.
diff --git a/proxmox-apt/src/repositories/file/sources_parser.rs b/proxmox-apt/src/repositories/file/sources_parser.rs
index 9424bbe2..213db9d6 100644
--- a/proxmox-apt/src/repositories/file/sources_parser.rs
+++ b/proxmox-apt/src/repositories/file/sources_parser.rs
@@ -3,6 +3,7 @@ use std::iter::Iterator;
use anyhow::{bail, Error};
+use crate::repositories::APTRepositoryImpl;
use crate::repositories::{
APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryPackageType,
};
diff --git a/proxmox-apt/src/repositories/mod.rs b/proxmox-apt/src/repositories/mod.rs
index 26e4192c..014f1820 100644
--- a/proxmox-apt/src/repositories/mod.rs
+++ b/proxmox-apt/src/repositories/mod.rs
@@ -4,17 +4,20 @@ use std::path::PathBuf;
use anyhow::{bail, Error};
mod repository;
+pub use repository::APTRepositoryImpl;
pub use repository::{
APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryPackageType,
};
mod file;
+pub use file::APTRepositoryFileImpl;
pub use file::{APTRepositoryFile, APTRepositoryFileError, APTRepositoryInfo};
mod release;
pub use release::{get_current_release_codename, DebianCodename};
mod standard;
+pub use standard::APTRepositoryHandleImpl;
pub use standard::{APTRepositoryHandle, APTStandardRepository};
const APT_SOURCES_LIST_FILENAME: &str = "/etc/apt/sources.list";
diff --git a/proxmox-apt/src/repositories/repository.rs b/proxmox-apt/src/repositories/repository.rs
index 91bd64f2..a07db7cb 100644
--- a/proxmox-apt/src/repositories/repository.rs
+++ b/proxmox-apt/src/repositories/repository.rs
@@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize};
use proxmox_schema::api;
use crate::repositories::standard::APTRepositoryHandle;
+use crate::repositories::standard::APTRepositoryHandleImpl;
#[api]
#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq, Eq)]
@@ -188,9 +189,41 @@ pub struct APTRepository {
pub enabled: bool,
}
-impl APTRepository {
+pub trait APTRepositoryImpl {
/// Crates an empty repository.
- pub fn new(file_type: APTRepositoryFileType) -> Self {
+ fn new(file_type: APTRepositoryFileType) -> Self;
+
+ /// Changes the `enabled` flag and makes sure the `Enabled` option for
+ /// `APTRepositoryPackageType::Sources` repositories is updated too.
+ fn set_enabled(&mut self, enabled: bool);
+
+ /// Makes sure that all basic properties of a repository are present and not obviously invalid.
+ fn basic_check(&self) -> Result<(), Error>;
+
+ /// Checks if the repository is the one referenced by the handle.
+ fn is_referenced_repository(
+ &self,
+ handle: APTRepositoryHandle,
+ product: &str,
+ suite: &str,
+ ) -> bool;
+
+ /// Guess the origin from the repository's URIs.
+ ///
+ /// Intended to be used as a fallback for get_cached_origin.
+ fn origin_from_uris(&self) -> Option<String>;
+
+ /// Get the `Origin:` value from a cached InRelease file.
+ fn get_cached_origin(&self) -> Result<Option<String>, Error>;
+
+ /// Writes a repository in the corresponding format followed by a blank.
+ ///
+ /// Expects that `basic_check()` for the repository was successful.
+ fn write(&self, w: &mut dyn Write) -> Result<(), Error>;
+}
+
+impl APTRepositoryImpl for APTRepository {
+ fn new(file_type: APTRepositoryFileType) -> Self {
Self {
types: vec![],
uris: vec![],
@@ -203,9 +236,7 @@ impl APTRepository {
}
}
- /// Changes the `enabled` flag and makes sure the `Enabled` option for
- /// `APTRepositoryPackageType::Sources` repositories is updated too.
- pub fn set_enabled(&mut self, enabled: bool) {
+ fn set_enabled(&mut self, enabled: bool) {
self.enabled = enabled;
if self.file_type == APTRepositoryFileType::Sources {
@@ -226,8 +257,7 @@ impl APTRepository {
}
}
- /// Makes sure that all basic properties of a repository are present and not obviously invalid.
- pub fn basic_check(&self) -> Result<(), Error> {
+ fn basic_check(&self) -> Result<(), Error> {
if self.types.is_empty() {
bail!("missing package type(s)");
}
@@ -267,8 +297,7 @@ impl APTRepository {
Ok(())
}
- /// Checks if the repository is the one referenced by the handle.
- pub fn is_referenced_repository(
+ fn is_referenced_repository(
&self,
handle: APTRepositoryHandle,
product: &str,
@@ -299,10 +328,7 @@ impl APTRepository {
&& found_component
}
- /// Guess the origin from the repository's URIs.
- ///
- /// Intended to be used as a fallback for get_cached_origin.
- pub fn origin_from_uris(&self) -> Option<String> {
+ fn origin_from_uris(&self) -> Option<String> {
for uri in self.uris.iter() {
if let Some(host) = host_from_uri(uri) {
if host == "proxmox.com" || host.ends_with(".proxmox.com") {
@@ -318,8 +344,7 @@ impl APTRepository {
None
}
- /// Get the `Origin:` value from a cached InRelease file.
- pub fn get_cached_origin(&self) -> Result<Option<String>, Error> {
+ fn get_cached_origin(&self) -> Result<Option<String>, Error> {
for uri in self.uris.iter() {
for suite in self.suites.iter() {
let mut file = release_filename(uri, suite, false);
@@ -353,10 +378,7 @@ impl APTRepository {
Ok(None)
}
- /// Writes a repository in the corresponding format followed by a blank.
- ///
- /// Expects that `basic_check()` for the repository was successful.
- pub fn write(&self, w: &mut dyn Write) -> Result<(), Error> {
+ fn write(&self, w: &mut dyn Write) -> Result<(), Error> {
match self.file_type {
APTRepositoryFileType::List => write_one_line(self, w),
APTRepositoryFileType::Sources => write_stanza(self, w),
diff --git a/proxmox-apt/src/repositories/standard.rs b/proxmox-apt/src/repositories/standard.rs
index 29cc7885..7858fac4 100644
--- a/proxmox-apt/src/repositories/standard.rs
+++ b/proxmox-apt/src/repositories/standard.rs
@@ -111,9 +111,26 @@ impl Display for APTRepositoryHandle {
}
}
-impl APTRepositoryHandle {
+pub trait APTRepositoryHandleImpl {
/// Get the description for the repository.
- pub fn description(self) -> String {
+ fn description(self) -> String;
+ /// Get the display name of the repository.
+ fn name(self) -> String;
+ /// Get the standard file path for the repository referenced by the handle.
+ fn path(self, product: &str) -> String;
+ /// Get package type, possible URIs and the component associated with the handle.
+ ///
+ /// The first URI is the preferred one.
+ fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String);
+ /// Get the standard repository referenced by the handle.
+ ///
+ /// An URI in the result is not '/'-terminated (under the assumption that no valid
+ /// product name is).
+ fn to_repository(self, product: &str, suite: &str) -> APTRepository;
+}
+
+impl APTRepositoryHandleImpl for APTRepositoryHandle {
+ fn description(self) -> String {
match self {
APTRepositoryHandle::Enterprise => {
"This is the default, stable, and recommended repository, available for all \
@@ -155,8 +172,7 @@ impl APTRepositoryHandle {
.to_string()
}
- /// Get the display name of the repository.
- pub fn name(self) -> String {
+ fn name(self) -> String {
match self {
APTRepositoryHandle::Enterprise => "Enterprise",
APTRepositoryHandle::NoSubscription => "No-Subscription",
@@ -171,8 +187,7 @@ impl APTRepositoryHandle {
.to_string()
}
- /// Get the standard file path for the repository referenced by the handle.
- pub fn path(self, product: &str) -> String {
+ fn path(self, product: &str) -> String {
match self {
APTRepositoryHandle::Enterprise => {
format!("/etc/apt/sources.list.d/{}-enterprise.list", product)
@@ -188,10 +203,7 @@ impl APTRepositoryHandle {
}
}
- /// Get package type, possible URIs and the component associated with the handle.
- ///
- /// The first URI is the preferred one.
- pub fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String) {
+ fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String) {
match self {
APTRepositoryHandle::Enterprise => (
APTRepositoryPackageType::Deb,
@@ -259,11 +271,7 @@ impl APTRepositoryHandle {
}
}
- /// Get the standard repository referenced by the handle.
- ///
- /// An URI in the result is not '/'-terminated (under the assumption that no valid
- /// product name is).
- pub fn to_repository(self, product: &str, suite: &str) -> APTRepository {
+ fn to_repository(self, product: &str, suite: &str) -> APTRepository {
let (package_type, uris, component) = self.info(product);
APTRepository {
diff --git a/proxmox-apt/tests/repositories.rs b/proxmox-apt/tests/repositories.rs
index ae92e51c..37d665bf 100644
--- a/proxmox-apt/tests/repositories.rs
+++ b/proxmox-apt/tests/repositories.rs
@@ -8,6 +8,9 @@ use proxmox_apt::repositories::{
check_repositories, get_current_release_codename, standard_repositories, APTRepositoryFile,
APTRepositoryHandle, APTRepositoryInfo, APTStandardRepository, DebianCodename,
};
+use proxmox_apt::repositories::{
+ APTRepositoryFileImpl, APTRepositoryHandleImpl, APTRepositoryImpl,
+};
fn create_clean_directory(path: &PathBuf) -> Result<(), Error> {
match std::fs::remove_dir_all(path) {
--
2.39.2
More information about the pbs-devel
mailing list