Re: [pbs-devel] [PATCH v3 pxar 3/6] format: add helper type ContentRange

[Christian Ebner <c.ebner@proxmox.com>]

On 6/12/24 11:27, Fabian Grünbichler wrote:
> On June 12, 2024 10:23 am, Christian Ebner wrote:
>> The new `ContentRange` type will be used to store content ranges to
>> be used when accessing a pxar archive via the decoder, but with
>> additional payload reference information in order to be able to
>> perform additional consistency checks on these entries.
> 
> this is only used in the accessor part, and is not really related to the
> format at all, so maybe let's move it there?

true, will be moved to the accessor

> 
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 2:
>> - limit fields to be pub(crate) only
>>
>>   src/format/mod.rs | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/src/format/mod.rs b/src/format/mod.rs
>> index 0648924..37ba99f 100644
>> --- a/src/format/mod.rs
>> +++ b/src/format/mod.rs
>> @@ -43,6 +43,7 @@ use std::fmt;
>>   use std::fmt::Display;
>>   use std::io;
>>   use std::mem::size_of;
>> +use std::ops::Range;
>>   use std::os::unix::ffi::OsStrExt;
>>   use std::path::Path;
>>   use std::time::{Duration, SystemTime};
>> @@ -844,3 +845,13 @@ pub(crate) fn check_payload_header_and_size(header: &Header, size: u64) -> io::R
>>   
>>       Ok(())
>>   }
>> +
>> +/// Stores a content range to be accessed via the `Accessor` as well as the payload reference to
>> +/// perform consistency checks on payload references for archives accessed via split variant input.
>> +#[derive(Clone)]
>> +pub struct ContentRange {
>> +    // Range of the content
>> +    pub(crate) content: Range<u64>,
>> +    // Optional payload ref
>> +    pub(crate) payload_ref: Option<PayloadRef>,
> 
> then these don't need to be pub(crate) either >
> some higher level questions:
> - this is effectively an opaque type then (the caller of
>    `content_range` gets it, but can then only pass it to
>    `open_contents_at_range`)
> - the range is derived directly from the entry, there's no longer a way
>    to pass in arbitrary ranges
> - what is the difference w.r.t. safety compared to `contents`
> related:
> - what about FileContentsImpl? it's returned by open_contents_at_range,
>    and takes an arbitrary range, should we instead give that ContentRange
>    as well while we're at it and breaking API?
> 
> is there actually a use case for the unsafe interfaces above outside of
> our own code that would warrant splitting the new ContentRange-based
> (possibly safe?) interface from the unsafe "arbitrary Range" one? or
> should we stick to one, but allow creating arbitrary ContentRanges? a
> user that really wants to read an arbitrary range can then just not set
> a PayloadRef and thus skip the header check?

You are right that `open_content_at_range` is now locked down and since 
a caller can only construct it via the corresponding `content_range` 
call, i suggest to drop the unsafe.

Yes, it seems to be possible to even move the payload check to the 
`FileContentsImpl` `new` method by passing the `PxarVariant` as input 
and make it private as well, not requiring to leak the range anywhere 
else and effectively making all self consistent. Then there is also no 
need to store the `ContentRange` in the `FileContentsImpl` as such, only 
keeping the `Range` there is fine as the check happens on construction.

Will do these modifications and send an updated version.

Thanks for the input!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel