all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [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