[pbs-devel] [PATCH backup] use BufReader/Writer for Files passed to serde_json::from_reader/writer

[pbs-devel] [PATCH backup] use BufReader/Writer for Files passed to serde_json::from_reader/writer

[Wolfgang Bumiller]

As serde_json will otherwise read files 1 byte at a time.
Writing is a bit better, but syntacitcal elements (quotes, braces,
commas) still often show up as single write syscalls, so use BufWriter
there as well.

Note that while we do store the file in the resulting objects, we do not
need to keep the buffered read/writers as we always `seek` to the
beginning on further file operations.

Reported-by: Mark Schouten <mark@tuxis.nl>
Link: https://lists.proxmox.com/pipermail/pbs-devel/2022-April/004909.html
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
@Mark: Hope the Reported-by tag is fine with you.

 proxmox-file-restore/src/block_driver_qemu.rs | 8 ++++----
 src/config/tfa.rs                             | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/proxmox-file-restore/src/block_driver_qemu.rs b/proxmox-file-restore/src/block_driver_qemu.rs
index 0176f7f1..ac61f917 100644
--- a/proxmox-file-restore/src/block_driver_qemu.rs
+++ b/proxmox-file-restore/src/block_driver_qemu.rs
@@ -1,7 +1,7 @@
 //! Block file access via a small QEMU restore VM using the PBS block driver in QEMU
 use std::collections::HashMap;
 use std::fs::{File, OpenOptions};
-use std::io::{prelude::*, SeekFrom};
+use std::io::{prelude::*, BufReader, BufWriter, SeekFrom};
 
 use anyhow::{bail, Error};
 use futures::FutureExt;
@@ -51,7 +51,7 @@ impl VMStateMap {
     fn load() -> Result<Self, Error> {
         let mut file = Self::open_file_raw(true)?;
         lock_file(&mut file, true, Some(std::time::Duration::from_secs(120)))?;
-        let map = serde_json::from_reader(&file).unwrap_or_default();
+        let map = serde_json::from_reader(BufReader::new(&mut file)).unwrap_or_default();
         Ok(Self { map, file })
     }
 
@@ -59,14 +59,14 @@ impl VMStateMap {
     /// shell auto-completion, for anything requiring consistency use load() !
     fn load_read_only() -> Result<HashMap<String, VMState>, Error> {
         let file = Self::open_file_raw(false)?;
-        Ok(serde_json::from_reader(&file).unwrap_or_default())
+        Ok(serde_json::from_reader(BufReader::new(file)).unwrap_or_default())
     }
 
     /// Write back a potentially modified state map, consuming the held lock
     fn write(mut self) -> Result<(), Error> {
         self.file.seek(SeekFrom::Start(0))?;
         self.file.set_len(0)?;
-        serde_json::to_writer(self.file, &self.map)?;
+        serde_json::to_writer(BufWriter::new(&mut self.file), &self.map)?;
 
         // drop ourselves including file lock
         Ok(())
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 790e0960..7120553c 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -40,7 +40,7 @@ pub fn read() -> Result<TfaConfig, Error> {
         Err(err) => return Err(err.into()),
     };
 
-    Ok(serde_json::from_reader(file)?)
+    Ok(serde_json::from_reader(io::BufReader::new(file))?)
 }
 
 pub(crate) fn webauthn_config_digest(config: &WebauthnConfig) -> Result<[u8; 32], Error> {
@@ -116,7 +116,7 @@ impl TfaUserChallengeData {
     fn save(mut self) -> Result<(), Error> {
         self.rewind()?;
 
-        serde_json::to_writer(&mut &self.lock, &self.inner).map_err(|err| {
+        serde_json::to_writer(io::BufWriter::new(&mut &self.lock), &self.inner).map_err(|err| {
             format_err!("failed to update challenge file {:?}: {}", self.path, err)
         })?;
 
@@ -293,7 +293,7 @@ impl proxmox_tfa::api::OpenUserChallengeData for UserAccess {
 
         proxmox_sys::fs::lock_file(&mut file, true, None)?;
 
-        let inner = serde_json::from_reader(&mut file).map_err(|err| {
+        let inner = serde_json::from_reader(io::BufReader::new(&mut file)).map_err(|err| {
             format_err!("failed to read challenge data for user {}: {}", userid, err)
         })?;
 
-- 
2.30.2

[pbs-devel] applied: [PATCH backup] use BufReader/Writer for Files passed to serde_json::from_reader/writer

[Thomas Lamprecht]

On 06.04.22 14:51, Wolfgang Bumiller wrote:
> As serde_json will otherwise read files 1 byte at a time.
> Writing is a bit better, but syntacitcal elements (quotes, braces,
> commas) still often show up as single write syscalls, so use BufWriter
> there as well.
> 
> Note that while we do store the file in the resulting objects, we do not
> need to keep the buffered read/writers as we always `seek` to the
> beginning on further file operations.
> 
> Reported-by: Mark Schouten <mark@tuxis.nl>
> Link: https://lists.proxmox.com/pipermail/pbs-devel/2022-April/004909.html
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> @Mark: Hope the Reported-by tag is fine with you.
> 
>  proxmox-file-restore/src/block_driver_qemu.rs | 8 ++++----
>  src/config/tfa.rs                             | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
>

applied, thanks!

Re: [pbs-devel] [PATCH backup] use BufReader/Writer for Files passed to serde_json::from_reader/writer

[Mark Schouten]

Hi Wolfgang,

Thanks for the quick response (even though I somehow missed it). Reported-by is fine with me!

— 
Mark Schouten, CTO
Tuxis B.V.
mark@tuxis.nl



> On 6 Apr 2022, at 14:51, Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
> As serde_json will otherwise read files 1 byte at a time.
> Writing is a bit better, but syntacitcal elements (quotes, braces,
> commas) still often show up as single write syscalls, so use BufWriter
> there as well.
> 
> Note that while we do store the file in the resulting objects, we do not
> need to keep the buffered read/writers as we always `seek` to the
> beginning on further file operations.
> 
> Reported-by: Mark Schouten <mark@tuxis.nl>
> Link: https://lists.proxmox.com/pipermail/pbs-devel/2022-April/004909.html
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> @Mark: Hope the Reported-by tag is fine with you.
> 
> proxmox-file-restore/src/block_driver_qemu.rs | 8 ++++----
> src/config/tfa.rs                             | 6 +++---
> 2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/proxmox-file-restore/src/block_driver_qemu.rs b/proxmox-file-restore/src/block_driver_qemu.rs
> index 0176f7f1..ac61f917 100644
> --- a/proxmox-file-restore/src/block_driver_qemu.rs
> +++ b/proxmox-file-restore/src/block_driver_qemu.rs
> @@ -1,7 +1,7 @@
> //! Block file access via a small QEMU restore VM using the PBS block driver in QEMU
> use std::collections::HashMap;
> use std::fs::{File, OpenOptions};
> -use std::io::{prelude::*, SeekFrom};
> +use std::io::{prelude::*, BufReader, BufWriter, SeekFrom};
> 
> use anyhow::{bail, Error};
> use futures::FutureExt;
> @@ -51,7 +51,7 @@ impl VMStateMap {
>     fn load() -> Result<Self, Error> {
>         let mut file = Self::open_file_raw(true)?;
>         lock_file(&mut file, true, Some(std::time::Duration::from_secs(120)))?;
> -        let map = serde_json::from_reader(&file).unwrap_or_default();
> +        let map = serde_json::from_reader(BufReader::new(&mut file)).unwrap_or_default();
>         Ok(Self { map, file })
>     }
> 
> @@ -59,14 +59,14 @@ impl VMStateMap {
>     /// shell auto-completion, for anything requiring consistency use load() !
>     fn load_read_only() -> Result<HashMap<String, VMState>, Error> {
>         let file = Self::open_file_raw(false)?;
> -        Ok(serde_json::from_reader(&file).unwrap_or_default())
> +        Ok(serde_json::from_reader(BufReader::new(file)).unwrap_or_default())
>     }
> 
>     /// Write back a potentially modified state map, consuming the held lock
>     fn write(mut self) -> Result<(), Error> {
>         self.file.seek(SeekFrom::Start(0))?;
>         self.file.set_len(0)?;
> -        serde_json::to_writer(self.file, &self.map)?;
> +        serde_json::to_writer(BufWriter::new(&mut self.file), &self.map)?;
> 
>         // drop ourselves including file lock
>         Ok(())
> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
> index 790e0960..7120553c 100644
> --- a/src/config/tfa.rs
> +++ b/src/config/tfa.rs
> @@ -40,7 +40,7 @@ pub fn read() -> Result<TfaConfig, Error> {
>         Err(err) => return Err(err.into()),
>     };
> 
> -    Ok(serde_json::from_reader(file)?)
> +    Ok(serde_json::from_reader(io::BufReader::new(file))?)
> }
> 
> pub(crate) fn webauthn_config_digest(config: &WebauthnConfig) -> Result<[u8; 32], Error> {
> @@ -116,7 +116,7 @@ impl TfaUserChallengeData {
>     fn save(mut self) -> Result<(), Error> {
>         self.rewind()?;
> 
> -        serde_json::to_writer(&mut &self.lock, &self.inner).map_err(|err| {
> +        serde_json::to_writer(io::BufWriter::new(&mut &self.lock), &self.inner).map_err(|err| {
>             format_err!("failed to update challenge file {:?}: {}", self.path, err)
>         })?;
> 
> @@ -293,7 +293,7 @@ impl proxmox_tfa::api::OpenUserChallengeData for UserAccess {
> 
>         proxmox_sys::fs::lock_file(&mut file, true, None)?;
> 
> -        let inner = serde_json::from_reader(&mut file).map_err(|err| {
> +        let inner = serde_json::from_reader(io::BufReader::new(&mut file)).map_err(|err| {
>             format_err!("failed to read challenge data for user {}: {}", userid, err)
>         })?;
> 
> -- 
> 2.30.2
> 
>