all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Dominik Csapak <d.csapak@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Wolfgang Bumiller <w.bumiller@proxmox.com>
Subject: Re: [pbs-devel] [PATCH pxar] fix `decode_entry` on special files
Date: Tue, 15 Dec 2020 12:08:53 +0100	[thread overview]
Message-ID: <17a50cdc-1fd8-cf2f-0061-0c99542b660b@proxmox.com> (raw)
In-Reply-To: <20201215103737.9957-1-w.bumiller@proxmox.com>

looks good AFAICT
fixes the problem with fifo/sockets

Tested-By: Dominik Csapak <d.csapak@proxmox.com>

On 12/15/20 11:37 AM, Wolfgang Bumiller wrote:
> When using the random accessor to access FIFOs or sockets,
> the ranged reader limits the data to only that entry, and
> the `decode_entry` will never see a `PAYLOAD` or
> `GOODBYE_TABLE` item to finish the entry.
> Instead, it'll reach EOF and we need to handle this.
> The accessor now tells the decoder to expect EOF as a valid
> condition for ending the entry.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> 
> The included testcase previously failed with
>      "failed to decode entry for fifo0"
> 
>   src/accessor/mod.rs  |  2 +-
>   src/decoder/mod.rs   | 52 +++++++++++++++++++++++++++++++++++---------
>   tests/simple/main.rs | 46 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 89 insertions(+), 11 deletions(-)
> 
> diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs
> index 0ab03a6..d02dc13 100644
> --- a/src/accessor/mod.rs
> +++ b/src/accessor/mod.rs
> @@ -229,7 +229,7 @@ async fn get_decoder<T: ReadAt>(
>       entry_range: Range<u64>,
>       path: PathBuf,
>   ) -> io::Result<DecoderImpl<SeqReadAtAdapter<T>>> {
> -    Ok(DecoderImpl::new_full(SeqReadAtAdapter::new(input, entry_range), path).await?)
> +    Ok(DecoderImpl::new_full(SeqReadAtAdapter::new(input, entry_range), path, true).await?)
>   }
>   
>   // NOTE: This performs the Decoder::read_next_item() behavior! Keep in mind when changing!
> diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs
> index fcc2dd9..2a5e79a 100644
> --- a/src/decoder/mod.rs
> +++ b/src/decoder/mod.rs
> @@ -155,6 +155,10 @@ pub(crate) struct DecoderImpl<T> {
>       path_lengths: Vec<usize>,
>       state: State,
>       with_goodbye_tables: bool,
> +
> +    /// The random access code uses decoders for sub-ranges which may not end in a `PAYLOAD` for
> +    /// entries like FIFOs or sockets, so there we explicitly allow an item to terminate with EOF.
> +    eof_after_entry: bool,
>   }
>   
>   enum State {
> @@ -191,14 +195,18 @@ pub(crate) enum ItemResult {
>   
>   impl<I: SeqRead> DecoderImpl<I> {
>       pub async fn new(input: I) -> io::Result<Self> {
> -        Self::new_full(input, "/".into()).await
> +        Self::new_full(input, "/".into(), false).await
>       }
>   
>       pub(crate) fn input(&self) -> &I {
>           &self.input
>       }
>   
> -    pub(crate) async fn new_full(input: I, path: PathBuf) -> io::Result<Self> {
> +    pub(crate) async fn new_full(
> +        input: I,
> +        path: PathBuf,
> +        eof_after_entry: bool,
> +    ) -> io::Result<Self> {
>           let this = DecoderImpl {
>               input,
>               current_header: unsafe { mem::zeroed() },
> @@ -210,6 +218,7 @@ impl<I: SeqRead> DecoderImpl<I> {
>               path_lengths: Vec::new(),
>               state: State::Begin,
>               with_goodbye_tables: false,
> +            eof_after_entry,
>           };
>   
>           // this.read_next_entry().await?;
> @@ -383,7 +392,14 @@ impl<I: SeqRead> DecoderImpl<I> {
>   
>               self.current_header = unsafe { mem::zeroed() };
>   
> -            while self.read_next_item().await? != ItemResult::Entry {}
> +            loop {
> +                match self.read_next_item_or_eof().await? {
> +                    Some(ItemResult::Entry) => break,
> +                    Some(ItemResult::Attribute) => continue,
> +                    None if self.eof_after_entry => break,
> +                    None => io_bail!("unexpected EOF in entry"),
> +                }
> +            }
>   
>               if self.entry.is_dir() {
>                   self.path_lengths
> @@ -402,24 +418,40 @@ impl<I: SeqRead> DecoderImpl<I> {
>               .ok_or_else(|| io_format_err!("unexpected EOF"))
>       }
>   
> +    async fn read_next_item(&mut self) -> io::Result<ItemResult> {
> +        match self.read_next_item_or_eof().await? {
> +            Some(item) => Ok(item),
> +            None => io_bail!("unexpected EOF"),
> +        }
> +    }
> +
> +    // NOTE: The random accessor will decode FIFOs and Sockets in a decoder instance with a ranged
> +    // reader so there is no PAYLOAD or GOODBYE TABLE to "end" an entry.
> +    //
>       // NOTE: This behavior method is also recreated in the accessor's `get_decoder_at_filename`
>       // function! Keep in mind when changing!
> -    async fn read_next_item(&mut self) -> io::Result<ItemResult> {
> -        self.read_next_header().await?;
> -        self.read_current_item().await
> +    async fn read_next_item_or_eof(&mut self) -> io::Result<Option<ItemResult>> {
> +        match self.read_next_header_or_eof().await? {
> +            Some(()) => self.read_current_item().await.map(Some),
> +            None => Ok(None),
> +        }
>       }
>   
> -    async fn read_next_header(&mut self) -> io::Result<()> {
> +    async fn read_next_header_or_eof(&mut self) -> io::Result<Option<()>> {
>           let dest = unsafe {
>               std::slice::from_raw_parts_mut(
>                   &mut self.current_header as *mut Header as *mut u8,
>                   size_of_val(&self.current_header),
>               )
>           };
> -        seq_read_exact(&mut self.input, dest).await?;
> -        self.current_header.check_header_size()?;
>   
> -        Ok(())
> +        match seq_read_exact_or_eof(&mut self.input, dest).await? {
> +            Some(()) => {
> +                self.current_header.check_header_size()?;
> +                Ok(Some(()))
> +            }
> +            None => Ok(None),
> +        }
>       }
>   
>       /// Read the next item, the header is already loaded.
> diff --git a/tests/simple/main.rs b/tests/simple/main.rs
> index c73ca10..f15a0f5 100644
> --- a/tests/simple/main.rs
> +++ b/tests/simple/main.rs
> @@ -42,6 +42,9 @@ fn test1() {
>   
>       assert!(!file.is_empty(), "encoder did not write any data");
>   
> +    // may be useful for testing...
> +    // std::fs::write("myarchive.pxar", &file).expect("failed to write out test archive");
> +
>       let mut input = &file[..];
>       let mut decoder = decoder::Decoder::from_std(&mut input).expect("failed to create decoder");
>       let decoded_fs =
> @@ -53,6 +56,7 @@ fn test1() {
>           .expect("failed to create random access reader for encoded archive");
>   
>       check_bunzip2(&accessor);
> +    check_run_special_files(&accessor);
>   }
>   
>   fn check_bunzip2(accessor: &accessor::Accessor<&[u8]>) {
> @@ -85,3 +89,45 @@ fn check_bunzip2(accessor: &accessor::Accessor<&[u8]>) {
>   
>       assert_eq!(content, "This is the bzip2 executable");
>   }
> +
> +fn check_run_special_files(accessor: &accessor::Accessor<&[u8]>) {
> +    let rundir = accessor
> +        .open_root()
> +        .expect("failed to open root of encoded archive")
> +        .lookup("run")
> +        .expect("failed to open /run in encoded archive")
> +        .expect("missing /run in encoded archive")
> +        .enter_directory()
> +        .expect("expected /run to be a directory in the test archive");
> +
> +    assert_eq!(rundir.entry_count(), 2, "expected 2 entries in /run");
> +
> +    let mut rd = rundir.read_dir();
> +    let fifo0 = rd
> +        .next()
> +        .expect("expected 'fifo0' entry in rundir")
> +        .expect("failed to get first (fifo0) entry in test archive /run directory");
> +    assert_eq!(
> +        fifo0.file_name(),
> +        Path::new("fifo0"),
> +        "expected first file in /run to be fifo0"
> +    );
> +
> +    let _entry = fifo0
> +        .decode_entry()
> +        .expect("failed to decode entry for fifo0");
> +
> +    let sock0 = rd
> +        .next()
> +        .expect("expected 'sock0' entry in rundir")
> +        .expect("failed to get second (sock0) entry in test archive /run directory");
> +    assert_eq!(
> +        sock0.file_name(),
> +        Path::new("sock0"),
> +        "expected second file in /run to be sock0"
> +    );
> +
> +    let _entry = sock0
> +        .decode_entry()
> +        .expect("failed to decode entry for sock0");
> +}
> 





  reply	other threads:[~2020-12-15 11:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-15 10:37 Wolfgang Bumiller
2020-12-15 11:08 ` Dominik Csapak [this message]
2020-12-15 12:18   ` [pbs-devel] applied: " Wolfgang Bumiller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=17a50cdc-1fd8-cf2f-0061-0c99542b660b@proxmox.com \
    --to=d.csapak@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=w.bumiller@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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