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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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?

> 
> 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?

> +}
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> 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