[pbs-devel] [PATCH offline-mirror] mirror: Use PathBuf instead of strings for paths

[pbs-devel] [PATCH offline-mirror] mirror: Use PathBuf instead of strings for paths

[Maximiliano Sandoval R]

Joining the strings might results in a double `//` in a path. This was
experienced in a ticket at our customer support in the following error:

    Error: unable to read
    "/var/lib/proxmox-offline-mirror/mirrors//.pool/sha256/<SOME_HASH>"
    - Input/output error (os error 5) after downloading ±60GB of data.

Suggested-by: Stefan Sterz <s.sterz@proxmox.com>
Signed-off-by: Maximiliano Sandoval R <m.sandoval@proxmox.com>
---

One could also store .base_dir and .id as PathBuf instead of Strings in
MirrorConfig but that would constitute an API break.

 src/mirror.rs | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/mirror.rs b/src/mirror.rs
index 3766f23..e26457b 100644
--- a/src/mirror.rs
+++ b/src/mirror.rs
@@ -30,13 +30,13 @@ use proxmox_apt::{
 
 use crate::helpers;
 
-fn mirror_dir(config: &MirrorConfig) -> String {
-    format!("{}/{}", config.base_dir, config.id)
+fn mirror_dir(config: &MirrorConfig) -> PathBuf {
+    PathBuf::from(&config.base_dir).join(&config.id)
 }
 
 pub(crate) fn pool(config: &MirrorConfig) -> Result<Pool, Error> {
-    let pool_dir = format!("{}/.pool", config.base_dir);
-    Pool::open(Path::new(&mirror_dir(config)), Path::new(&pool_dir))
+    let pool_dir = PathBuf::from(&config.base_dir).join(".pool");
+    Pool::open(&mirror_dir(config), &pool_dir)
 }
 
 /// `MirrorConfig`, but some fields converted/parsed into usable types.
@@ -450,11 +450,11 @@ fn fetch_plain_file(
 
 /// Initialize a new mirror (by creating the corresponding pool).
 pub fn init(config: &MirrorConfig) -> Result<(), Error> {
-    let pool_dir = format!("{}/.pool", config.base_dir);
+    let pool_dir = PathBuf::from(&config.base_dir).join(".pool");
 
-    let dir = format!("{}/{}", config.base_dir, config.id);
+    let dir = PathBuf::from(&config.base_dir).join(&config.id);
 
-    Pool::create(Path::new(&dir), Path::new(&pool_dir))?;
+    Pool::create(&dir, &pool_dir)?;
     Ok(())
 }
 
@@ -472,13 +472,11 @@ pub fn list_snapshots(config: &MirrorConfig) -> Result<Vec<Snapshot>, Error> {
 
     let mut list: Vec<Snapshot> = vec![];
 
-    let dir = mirror_dir(config);
-
-    let path = Path::new(&dir);
+    let path = mirror_dir(config);
 
     proxmox_sys::fs::scandir(
         libc::AT_FDCWD,
-        path,
+        &path,
         &SNAPSHOT_REGEX,
         |_l2_fd, snapshot, file_type| {
             if file_type != nix::dir::Type::Directory {
-- 
2.39.2

Re: [pbs-devel] [PATCH offline-mirror] mirror: Use PathBuf instead of strings for paths

[Thomas Lamprecht]

Am 21/11/2023 um 14:50 schrieb Maximiliano Sandoval R:
> Joining the strings might results in a double `//` in a path. This was
> experienced in a ticket at our customer support in the following error:
> 
>     Error: unable to read
>     "/var/lib/proxmox-offline-mirror/mirrors//.pool/sha256/<SOME_HASH>"
>     - Input/output error (os error 5) after downloading ±60GB of data.


I mean, that's an example where one can view this slightly cosmetic issue,
not really the cause for the actual error, like your message tends to
suggest, I hope?!

Re: [pbs-devel] [PATCH offline-mirror] mirror: Use PathBuf instead of strings for paths

[Maximiliano Sandoval]

Yes, it ends up being mostly cosmetic.

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> Am 21/11/2023 um 14:50 schrieb Maximiliano Sandoval R:
>> Joining the strings might results in a double `//` in a path. This was
>> experienced in a ticket at our customer support in the following error:
>>
>>     Error: unable to read
>>     "/var/lib/proxmox-offline-mirror/mirrors//.pool/sha256/<SOME_HASH>"
>>     - Input/output error (os error 5) after downloading ±60GB of data.
>
>
> I mean, that's an example where one can view this slightly cosmetic issue,
> not really the cause for the actual error, like your message tends to
> suggest, I hope?!


--
Maximiliano

[pbs-devel] applied: [PATCH offline-mirror] mirror: Use PathBuf instead of strings for paths

[Fabian Grünbichler]

with a small follow-up to replace another instance of `mirror_dir`'s
code with a call to it ;) 

On November 21, 2023 2:50 pm, Maximiliano Sandoval R wrote:
> Joining the strings might results in a double `//` in a path. This was
> experienced in a ticket at our customer support in the following error:
> 
>     Error: unable to read
>     "/var/lib/proxmox-offline-mirror/mirrors//.pool/sha256/<SOME_HASH>"
>     - Input/output error (os error 5) after downloading ±60GB of data.
> 
> Suggested-by: Stefan Sterz <s.sterz@proxmox.com>
> Signed-off-by: Maximiliano Sandoval R <m.sandoval@proxmox.com>
> ---
> 
> One could also store .base_dir and .id as PathBuf instead of Strings in
> MirrorConfig but that would constitute an API break.
> 
>  src/mirror.rs | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mirror.rs b/src/mirror.rs
> index 3766f23..e26457b 100644
> --- a/src/mirror.rs
> +++ b/src/mirror.rs
> @@ -30,13 +30,13 @@ use proxmox_apt::{
>  
>  use crate::helpers;
>  
> -fn mirror_dir(config: &MirrorConfig) -> String {
> -    format!("{}/{}", config.base_dir, config.id)
> +fn mirror_dir(config: &MirrorConfig) -> PathBuf {
> +    PathBuf::from(&config.base_dir).join(&config.id)
>  }
>  
>  pub(crate) fn pool(config: &MirrorConfig) -> Result<Pool, Error> {
> -    let pool_dir = format!("{}/.pool", config.base_dir);
> -    Pool::open(Path::new(&mirror_dir(config)), Path::new(&pool_dir))
> +    let pool_dir = PathBuf::from(&config.base_dir).join(".pool");
> +    Pool::open(&mirror_dir(config), &pool_dir)
>  }
>  
>  /// `MirrorConfig`, but some fields converted/parsed into usable types.
> @@ -450,11 +450,11 @@ fn fetch_plain_file(
>  
>  /// Initialize a new mirror (by creating the corresponding pool).
>  pub fn init(config: &MirrorConfig) -> Result<(), Error> {
> -    let pool_dir = format!("{}/.pool", config.base_dir);
> +    let pool_dir = PathBuf::from(&config.base_dir).join(".pool");
>  
> -    let dir = format!("{}/{}", config.base_dir, config.id);
> +    let dir = PathBuf::from(&config.base_dir).join(&config.id);
>  
> -    Pool::create(Path::new(&dir), Path::new(&pool_dir))?;
> +    Pool::create(&dir, &pool_dir)?;
>      Ok(())
>  }
>  
> @@ -472,13 +472,11 @@ pub fn list_snapshots(config: &MirrorConfig) -> Result<Vec<Snapshot>, Error> {
>  
>      let mut list: Vec<Snapshot> = vec![];
>  
> -    let dir = mirror_dir(config);
> -
> -    let path = Path::new(&dir);
> +    let path = mirror_dir(config);
>  
>      proxmox_sys::fs::scandir(
>          libc::AT_FDCWD,
> -        path,
> +        &path,
>          &SNAPSHOT_REGEX,
>          |_l2_fd, snapshot, file_type| {
>              if file_type != nix::dir::Type::Directory {
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>