* [pbs-devel] [PATCH backup] use truncate whenever we create files
@ 2024-12-02 13:17 Maximiliano Sandoval
2024-12-03 10:40 ` Fabian Grünbichler
0 siblings, 1 reply; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-12-02 13:17 UTC (permalink / raw)
To: pbs-devel
Fixes the suspicious_open_options clippy lint, for example:
```
warning: file opened with `create`, but `truncate` behavior not defined
--> src/api2/tape/restore.rs:1713:18
|
1713 | .create(true)
| ^^^^^^^^^^^^- help: add: `.truncate(true)`
|
= help: if you intend to overwrite an existing file entirely, call `.truncate(true)`
= help: if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
= help: alternatively, use `.append(true)` to append to the file instead of overwriting it
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_open_options
```
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
Some of these are quite obvious (tmp files) but I am not 100% about all of them,
a few extra eyes on it would ne appreciated.
pbs-client/src/pxar/extract.rs | 1 +
proxmox-backup-client/src/main.rs | 2 ++
src/api2/tape/restore.rs | 1 +
src/server/sync.rs | 1 +
4 files changed, 5 insertions(+)
diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index cfbd8d51..b3e7d535 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -134,6 +134,7 @@ where
if let Some(entry) = prelude {
let mut prelude_file = OpenOptions::new()
.create(true)
+ .truncate(true)
.write(true)
.open(path)
.with_context(|| format!("error creating prelude file '{path:?}'"))?;
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index b0d45f89..9fcf35d7 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -1597,6 +1597,7 @@ async fn restore(
let mut writer = std::fs::OpenOptions::new()
.write(true)
.create(true)
+ .truncate(true)
.create_new(true)
.open(target)
.map_err(|err| {
@@ -1723,6 +1724,7 @@ async fn restore(
std::fs::OpenOptions::new()
.write(true)
.create(true)
+ .truncate(true)
.create_new(true)
.open(target)
.map_err(|err| format_err!("unable to create target file {:?} - {}", target, err))?
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 93a6053b..2cc1baab 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -1711,6 +1711,7 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
let mut tmpfile = std::fs::OpenOptions::new()
.write(true)
.create(true)
+ .truncate(true)
.read(true)
.open(&tmp_path)
.map_err(|err| format_err!("restore {:?} failed - {}", tmp_path, err))?;
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 0bd7a7a8..04654c28 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -159,6 +159,7 @@ impl SyncSourceReader for RemoteSourceReader {
let tmpfile = std::fs::OpenOptions::new()
.write(true)
.create(true)
+ .truncate(true)
.read(true)
.open(&tmp_path)?;
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH backup] use truncate whenever we create files
2024-12-02 13:17 [pbs-devel] [PATCH backup] use truncate whenever we create files Maximiliano Sandoval
@ 2024-12-03 10:40 ` Fabian Grünbichler
2025-01-15 15:20 ` Maximiliano Sandoval
0 siblings, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2024-12-03 10:40 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On December 2, 2024 2:17 pm, Maximiliano Sandoval wrote:
> Fixes the suspicious_open_options clippy lint, for example:
>
> ```
> warning: file opened with `create`, but `truncate` behavior not defined
> --> src/api2/tape/restore.rs:1713:18
> |
> 1713 | .create(true)
> | ^^^^^^^^^^^^- help: add: `.truncate(true)`
> |
> = help: if you intend to overwrite an existing file entirely, call `.truncate(true)`
> = help: if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
> = help: alternatively, use `.append(true)` to append to the file instead of overwriting it
> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_open_options
> ```
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>
> Some of these are quite obvious (tmp files) but I am not 100% about all of them,
> a few extra eyes on it would ne appreciated.
>
>
> pbs-client/src/pxar/extract.rs | 1 +
> proxmox-backup-client/src/main.rs | 2 ++
> src/api2/tape/restore.rs | 1 +
> src/server/sync.rs | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
> index cfbd8d51..b3e7d535 100644
> --- a/pbs-client/src/pxar/extract.rs
> +++ b/pbs-client/src/pxar/extract.rs
> @@ -134,6 +134,7 @@ where
> if let Some(entry) = prelude {
> let mut prelude_file = OpenOptions::new()
> .create(true)
> + .truncate(true)
this one should honor the overwrite flag and not truncate
unconditionally (see below)
> .write(true)
> .open(path)
> .with_context(|| format!("error creating prelude file '{path:?}'"))?;
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index b0d45f89..9fcf35d7 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -1597,6 +1597,7 @@ async fn restore(
> let mut writer = std::fs::OpenOptions::new()
> .write(true)
> .create(true)
> + .truncate(true)
> .create_new(true)
truncate/create and create_new don't make any sense? IMHO this should
probably also honor the overwrite flag instead?
if we want to overwrite, I think setting create+truncate would be
correct, if we don't want to overwrite, only setting create_new would be
the way to go
https://doc.rust-lang.org/stable/src/std/sys/pal/unix/fs.rs.html#1144
> .open(target)
> .map_err(|err| {
> @@ -1723,6 +1724,7 @@ async fn restore(
> std::fs::OpenOptions::new()
> .write(true)
> .create(true)
> + .truncate(true)
> .create_new(true)
same here
> .open(target)
> .map_err(|err| format_err!("unable to create target file {:?} - {}", target, err))?
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 93a6053b..2cc1baab 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -1711,6 +1711,7 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
> let mut tmpfile = std::fs::OpenOptions::new()
> .write(true)
> .create(true)
> + .truncate(true)
this is okay, provided the tape code has locked the snapshot path this
is operating in (seems to be the case after a quick glance - @Dominik?)
> .read(true)
> .open(&tmp_path)
> .map_err(|err| format_err!("restore {:?} failed - {}", tmp_path, err))?;
> diff --git a/src/server/sync.rs b/src/server/sync.rs
> index 0bd7a7a8..04654c28 100644
> --- a/src/server/sync.rs
> +++ b/src/server/sync.rs
> @@ -159,6 +159,7 @@ impl SyncSourceReader for RemoteSourceReader {
> let tmpfile = std::fs::OpenOptions::new()
> .write(true)
> .create(true)
> + .truncate(true)
this is in a locked context, so should be okay as well..
> .read(true)
> .open(&tmp_path)?;
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH backup] use truncate whenever we create files
2024-12-03 10:40 ` Fabian Grünbichler
@ 2025-01-15 15:20 ` Maximiliano Sandoval
2025-01-27 9:24 ` Maximiliano Sandoval
0 siblings, 1 reply; 4+ messages in thread
From: Maximiliano Sandoval @ 2025-01-15 15:20 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:
>> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
>> index cfbd8d51..b3e7d535 100644
>> --- a/pbs-client/src/pxar/extract.rs
>> +++ b/pbs-client/src/pxar/extract.rs
>> @@ -134,6 +134,7 @@ where
>> if let Some(entry) = prelude {
>> let mut prelude_file = OpenOptions::new()
>> .create(true)
>> + .truncate(true)
>> .write(true)
>> .open(path)
>> .with_context(|| format!("error creating prelude file '{path:?}'"))?;
>
> this one should honor the overwrite flag and not truncate
> unconditionally (see below)
Sorry I am not following, the two diffs bellow use the same flags
(except that they also add `read(true)`).
>> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
>> index 93a6053b..2cc1baab 100644
>> --- a/src/api2/tape/restore.rs
>> +++ b/src/api2/tape/restore.rs
>> @@ -1711,6 +1711,7 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
>> let mut tmpfile = std::fs::OpenOptions::new()
>> .write(true)
>> .create(true)
>> + .truncate(true)
>
> this is okay, provided the tape code has locked the snapshot path this
> is operating in (seems to be the case after a quick glance - @Dominik?)
>
>> .read(true)
>> .open(&tmp_path)
>> .map_err(|err| format_err!("restore {:?} failed - {}", tmp_path, err))?;
>> diff --git a/src/server/sync.rs b/src/server/sync.rs
>> index 0bd7a7a8..04654c28 100644
>> --- a/src/server/sync.rs
>> +++ b/src/server/sync.rs
>> @@ -159,6 +159,7 @@ impl SyncSourceReader for RemoteSourceReader {
>> let tmpfile = std::fs::OpenOptions::new()
>> .write(true)
>> .create(true)
>> + .truncate(true)
>
> this is in a locked context, so should be okay as well..
>
>> .read(true)
>> .open(&tmp_path)?;
>>
>> --
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH backup] use truncate whenever we create files
2025-01-15 15:20 ` Maximiliano Sandoval
@ 2025-01-27 9:24 ` Maximiliano Sandoval
0 siblings, 0 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2025-01-27 9:24 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
> Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:
>
>>> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
>>> index cfbd8d51..b3e7d535 100644
>>> --- a/pbs-client/src/pxar/extract.rs
>>> +++ b/pbs-client/src/pxar/extract.rs
>>> @@ -134,6 +134,7 @@ where
>>> if let Some(entry) = prelude {
>>> let mut prelude_file = OpenOptions::new()
>>> .create(true)
>>> + .truncate(true)
>>> .write(true)
>>> .open(path)
>>> .with_context(|| format!("error creating prelude file '{path:?}'"))?;
>>
>> this one should honor the overwrite flag and not truncate
>> unconditionally (see below)
>
> Sorry I am not following, the two diffs bellow use the same flags
> (except that they also add `read(true)`).
>
>>> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
>>> index 93a6053b..2cc1baab 100644
>>> --- a/src/api2/tape/restore.rs
>>> +++ b/src/api2/tape/restore.rs
>>> @@ -1711,6 +1711,7 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
>>> let mut tmpfile = std::fs::OpenOptions::new()
>>> .write(true)
>>> .create(true)
>>> + .truncate(true)
>>
>> this is okay, provided the tape code has locked the snapshot path this
>> is operating in (seems to be the case after a quick glance - @Dominik?)
>>
>>> .read(true)
>>> .open(&tmp_path)
>>> .map_err(|err| format_err!("restore {:?} failed - {}", tmp_path, err))?;
>>> diff --git a/src/server/sync.rs b/src/server/sync.rs
>>> index 0bd7a7a8..04654c28 100644
>>> --- a/src/server/sync.rs
>>> +++ b/src/server/sync.rs
>>> @@ -159,6 +159,7 @@ impl SyncSourceReader for RemoteSourceReader {
>>> let tmpfile = std::fs::OpenOptions::new()
>>> .write(true)
>>> .create(true)
>>> + .truncate(true)
>>
>> this is in a locked context, so should be okay as well..
>>
>>> .read(true)
>>> .open(&tmp_path)?;
>>>
>>> --
>>> 2.39.5
>>>
>>>
>>>
>>> _______________________________________________
>>> pbs-devel mailing list
>>> pbs-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
v2 send.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-27 9:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-02 13:17 [pbs-devel] [PATCH backup] use truncate whenever we create files Maximiliano Sandoval
2024-12-03 10:40 ` Fabian Grünbichler
2025-01-15 15:20 ` Maximiliano Sandoval
2025-01-27 9:24 ` Maximiliano Sandoval
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal