* Re: [pbs-devel] [PATCH proxmox-backup v2 09/14] tape/*: clippy fixes
@ 2021-04-19 8:57 Wolfgang Bumiller
0 siblings, 0 replies; 2+ messages in thread
From: Wolfgang Bumiller @ 2021-04-19 8:57 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
> On 04/16/2021 12:29 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
>
>
> fixes:
> * absurd extreme comparisons
> * or_fun_call
> * unnecessary return
> * collapsible if chain
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/tape/file_formats/blocked_reader.rs | 4 ++--
> src/tape/file_formats/chunk_archive.rs | 12 +++++-------
> src/tape/file_formats/multi_volume_writer.rs | 2 +-
> src/tape/inventory.rs | 2 +-
> 4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/src/tape/file_formats/blocked_reader.rs b/src/tape/file_formats/blocked_reader.rs
> index ce2fc9aa..bfdbc2e7 100644
> --- a/src/tape/file_formats/blocked_reader.rs
> +++ b/src/tape/file_formats/blocked_reader.rs
> @@ -118,13 +118,13 @@ impl <R: BlockRead> BlockedReader<R> {
> proxmox::io_bail!("detected tape block after block-stream end marker");
> }
> Err(BlockReadError::EndOfFile) => {
> - return Ok(());
> + Ok(())
> }
> Err(BlockReadError::EndOfStream) => {
> proxmox::io_bail!("got unexpected end of tape");
> }
> Err(BlockReadError::Error(err)) => {
> - return Err(err);
> + Err(err)
> }
> }
> }
> diff --git a/src/tape/file_formats/chunk_archive.rs b/src/tape/file_formats/chunk_archive.rs
> index f6b07806..ab4234dd 100644
> --- a/src/tape/file_formats/chunk_archive.rs
> +++ b/src/tape/file_formats/chunk_archive.rs
> @@ -120,13 +120,11 @@ impl <'a> ChunkArchiveWriter<'a> {
> } else {
> self.write_all(&blob_data[start..end])?
> };
> - if leom {
> - if self.close_on_leom {
> - let mut writer = self.writer.take().unwrap();
> - writer.finish(false)?;
> - self.bytes_written = writer.bytes_written();
> - return Ok(chunk_is_complete);
> - }
> + if leom && self.close_on_leom {
> + let mut writer = self.writer.take().unwrap();
> + writer.finish(false)?;
> + self.bytes_written = writer.bytes_written();
> + return Ok(chunk_is_complete);
> }
> start = end;
> }
> diff --git a/src/tape/file_formats/multi_volume_writer.rs b/src/tape/file_formats/multi_volume_writer.rs
> index d58285e9..8e0e1d94 100644
> --- a/src/tape/file_formats/multi_volume_writer.rs
> +++ b/src/tape/file_formats/multi_volume_writer.rs
> @@ -72,7 +72,7 @@ impl <'a> TapeWrite for MultiVolumeWriter<'a> {
> }
>
> if self.writer.is_none() {
> - if self.header.part_number >= 255 {
> + if self.header.part_number == 255 {
So this caught my attention.
This method increments this to at most 255, which makes sense, given that this is an u8.
(But the incrementation & check could be closer together, be a separate method, or use .checked_add()
But what I'm worried about is the reader side where we have:
let expect_part_number = self.header.part_number + 1;
This should probably also sanity-check this possible overflow in case of bad/corrupted/fabricated data, no?
So if `255` is a valid number
> proxmox::io_bail!("multi-volume writer: too many parts");
> }
> self.writer = Some(
> diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
> index f9654538..15a51e5e 100644
> --- a/src/tape/inventory.rs
> +++ b/src/tape/inventory.rs
> @@ -792,7 +792,7 @@ pub fn lock_media_set(
> path.push(format!(".media-set-{}", media_set_uuid));
> path.set_extension("lck");
>
> - let timeout = timeout.unwrap_or(Duration::new(10, 0));
> + let timeout = timeout.unwrap_or_else(|| Duration::new(10, 0));
Oof, why is `Duration::new` not a `const fn` O.o
> let file = open_file_locked(&path, timeout, true)?;
> if cfg!(test) {
> // We cannot use chown inside test environment (no permissions)
> --
> 2.20.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 09/14] tape/*: clippy fixes
2021-04-16 10:28 [pbs-devel] [PATCH proxmox-backup v2 00/14] various " Dominik Csapak
@ 2021-04-16 10:29 ` Dominik Csapak
0 siblings, 0 replies; 2+ messages in thread
From: Dominik Csapak @ 2021-04-16 10:29 UTC (permalink / raw)
To: pbs-devel
fixes:
* absurd extreme comparisons
* or_fun_call
* unnecessary return
* collapsible if chain
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/tape/file_formats/blocked_reader.rs | 4 ++--
src/tape/file_formats/chunk_archive.rs | 12 +++++-------
src/tape/file_formats/multi_volume_writer.rs | 2 +-
src/tape/inventory.rs | 2 +-
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/tape/file_formats/blocked_reader.rs b/src/tape/file_formats/blocked_reader.rs
index ce2fc9aa..bfdbc2e7 100644
--- a/src/tape/file_formats/blocked_reader.rs
+++ b/src/tape/file_formats/blocked_reader.rs
@@ -118,13 +118,13 @@ impl <R: BlockRead> BlockedReader<R> {
proxmox::io_bail!("detected tape block after block-stream end marker");
}
Err(BlockReadError::EndOfFile) => {
- return Ok(());
+ Ok(())
}
Err(BlockReadError::EndOfStream) => {
proxmox::io_bail!("got unexpected end of tape");
}
Err(BlockReadError::Error(err)) => {
- return Err(err);
+ Err(err)
}
}
}
diff --git a/src/tape/file_formats/chunk_archive.rs b/src/tape/file_formats/chunk_archive.rs
index f6b07806..ab4234dd 100644
--- a/src/tape/file_formats/chunk_archive.rs
+++ b/src/tape/file_formats/chunk_archive.rs
@@ -120,13 +120,11 @@ impl <'a> ChunkArchiveWriter<'a> {
} else {
self.write_all(&blob_data[start..end])?
};
- if leom {
- if self.close_on_leom {
- let mut writer = self.writer.take().unwrap();
- writer.finish(false)?;
- self.bytes_written = writer.bytes_written();
- return Ok(chunk_is_complete);
- }
+ if leom && self.close_on_leom {
+ let mut writer = self.writer.take().unwrap();
+ writer.finish(false)?;
+ self.bytes_written = writer.bytes_written();
+ return Ok(chunk_is_complete);
}
start = end;
}
diff --git a/src/tape/file_formats/multi_volume_writer.rs b/src/tape/file_formats/multi_volume_writer.rs
index d58285e9..8e0e1d94 100644
--- a/src/tape/file_formats/multi_volume_writer.rs
+++ b/src/tape/file_formats/multi_volume_writer.rs
@@ -72,7 +72,7 @@ impl <'a> TapeWrite for MultiVolumeWriter<'a> {
}
if self.writer.is_none() {
- if self.header.part_number >= 255 {
+ if self.header.part_number == 255 {
proxmox::io_bail!("multi-volume writer: too many parts");
}
self.writer = Some(
diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
index f9654538..15a51e5e 100644
--- a/src/tape/inventory.rs
+++ b/src/tape/inventory.rs
@@ -792,7 +792,7 @@ pub fn lock_media_set(
path.push(format!(".media-set-{}", media_set_uuid));
path.set_extension("lck");
- let timeout = timeout.unwrap_or(Duration::new(10, 0));
+ let timeout = timeout.unwrap_or_else(|| Duration::new(10, 0));
let file = open_file_locked(&path, timeout, true)?;
if cfg!(test) {
// We cannot use chown inside test environment (no permissions)
--
2.20.1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-04-19 8:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 8:57 [pbs-devel] [PATCH proxmox-backup v2 09/14] tape/*: clippy fixes Wolfgang Bumiller
-- strict thread matches above, loose matches on Subject: below --
2021-04-16 10:28 [pbs-devel] [PATCH proxmox-backup v2 00/14] various " Dominik Csapak
2021-04-16 10:29 ` [pbs-devel] [PATCH proxmox-backup v2 09/14] tape/*: " Dominik Csapak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox