Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribute to handle length of bytes(or bits) in Vec<T> with other struct field #76

Closed
wcampbell0x2a opened this issue Jul 29, 2020 · 17 comments
Labels
enhancement New feature or request

Comments

@wcampbell0x2a
Copy link
Collaborator

wcampbell0x2a commented Jul 29, 2020

Consider the following code:

use deku::prelude::*;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
pub struct Packet {
    #[deku(bytes = "1")]
    length: u8,
    // byte len of all of messages is length - 2
    messages: Vec<Message>,
}

/// In the real packet, this would be of variable length, so we can't use just the `count` attribute on messages
#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
pub struct Message {
    #[deku(bytes = "1")]
    msg: u8,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test01() {
        let data: Vec<u8> = vec![0x04, 0x01, 0x02];

        let (_, value) = Packet::from_bytes((data.as_ref(), 0)).unwrap();

        assert_eq!(
            Packet {
                length: 0x04,
                messages: vec![Message { msg: 0x01 }, Message { msg: 0x02 }]
            },
            value
        );
    }
}

I can use the count attribute to give the length of Vec<T> elements, but I see no way of telling the max bytes that a Vec can have in it's container as a whole. Wondering if this would be a feature, or do I need to do some weird custom write/read implementation with a read_bytes field.

@sharksforarms
Copy link
Owner

Just to be clear, #[deku(count = "length - 2")] on messages doesn't satisfy this?

@wcampbell0x2a
Copy link
Collaborator Author

Just to be clear, #[deku(count = "length - 2")] on messages doesn't satisfy this?

Correct, since that is the length of elements in the Vec.

@constfold
Copy link
Contributor

Does ctx satisfy this situation?

struct A {
    len: u8,
    #[deku(ctx = "len")]
    messages: Vec<Message>
}
#[deku(ctx = "byte_len: u8")]
struct Message {
    #[deku(bytes = "byte_len")]
    msg: u8,
}

@wcampbell0x2a
Copy link
Collaborator Author

I don't think so, since it's not the length of the bytes in one Message, it's the length of All the Messages in a Vec.

Reminder that

However, having the following issues when testing your code:

use deku::prelude::*;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
pub struct Packet {
    len: u8,
    // byte len of all of messages is length - 2
    #[deku(ctx = "len")]
    messages: Vec<Message>,
}

/// In the real packet, this would be of variable length, so we can't use just the `count` attribute on messages
#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "byte_len: u8")]
pub struct Message {
    #[deku(bytes = "byte_len")]
    msg: u8,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test01() {
        let data: Vec<u8> = vec![0x04, 0x01, 0x02];

        let (_, value) = Packet::from_bytes((data.as_ref(), 0)).unwrap();

        assert_eq!(
            Packet {
                len: 0x04,
                messages: vec![Message { msg: 0x01 }, Message { msg: 0x02 }]
            },
            value
        );
    }
}

errors:

error: Unknown literal value `byte_len`
  --> src/main.rs:15:20
   |
15 |     #[deku(bytes = "byte_len")]
   |                    ^^^^^^^^^^

error[E0277]: the trait bound `std::vec::Vec<Message>: deku::DekuRead<&u8>` is not satisfied
 --> src/main.rs:3:28
  |
3 | #[derive(Debug, PartialEq, DekuRead, DekuWrite)]
  |                            ^^^^^^^^ the trait `deku::DekuRead<&u8>` is not implemented for `std::vec::Vec<Message>`
  |
  = help: the following implementations were found:
            <std::vec::Vec<T> as deku::DekuRead<(deku::ctx::Count, Ctx)>>
            <std::vec::Vec<T> as deku::DekuRead<deku::ctx::Count>>
  = note: required by `deku::DekuRead::read`
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `write` found for reference `&std::vec::Vec<Message>` in the current scope
   --> src/main.rs:3:38
    |
3   |   #[derive(Debug, PartialEq, DekuRead, DekuWrite)]
    |                                        ^^^^^^^^^ method not found in `&std::vec::Vec<Message>`
...
14  |   pub struct Message {
    |   ------------------ doesn't satisfy `Message: deku::DekuWrite<_>`
    |
    = note: the method `write` exists but the following trait bounds were not satisfied:
            `Message: deku::DekuWrite<_>`
            which is required by `std::vec::Vec<Message>: deku::DekuWrite<_>`
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `deku-example`.

@constfold
Copy link
Contributor

However, having the following issues when testing your code:

Yeah, that's just a demo.

the max bytes that a Vec can have in it's container as a whole.

hmm, the max bytes that Vec can have is count * sizeof(one_element)? Maybe I didn't get the point?

@wcampbell0x2a
Copy link
Collaborator Author

hmm, the max bytes that Vec can have is count * sizeof(one_element)? Maybe I didn't get the point?

I am just trying to figure out a way of parsing Message into a Vec until the length of the parsed bytes is equal to len - 2

@constfold
Copy link
Contributor

constfold commented Jul 29, 2020

Ah, understood. Currently there is no way to do this.
Maybe we could simply add a exhaustive version of Vec which doesn't require a Count, then you can write:

pub struct Packet {
    #[deku(bytes = "1")]
    length: u8,
    // Read all required bytes, then parse it.
    #[deku(
        count = "length - 2",
        map = "|all_msg: Vec<u8>|Vec::<Message>::try_from(all_msg)"
    )]
    messages: Vec<Message>,
}

Anyway, I agree that this feature should be added.

@sharksforarms
Copy link
Owner

sharksforarms commented Jul 29, 2020

It might help to provide a more concise example to work off of... is there a header format you're trying to parse? What is the advantage to "Read all requred bytes, then parse it" ?

@wcampbell0x2a
Copy link
Collaborator Author

In the asterix protocol, on page 24. It has a listed two-octet field LEN that gives the total length in octets of the record (Message in my example)

Notice the variable length of Data Fields depending on the fspec defined.

https://www.eurocontrol.int/sites/default/files/2019-06/part_1_-_eurocontrol_specification_asterix_spec-149_ed_2.4.pdf

@sharksforarms
Copy link
Owner

I understand now, thanks! Kinda sounds similar to reading TCP Options, I have used a custom reader for this: https://github.com/sharksforarms/rust-packet/blob/master/src/layer/tcp/mod.rs#L187

I agree this feature would be usefull.

@sharksforarms sharksforarms added the enhancement New feature or request label Jul 29, 2020
@sharksforarms
Copy link
Owner

Would something like this make sense? Allowing the use of bytes/bits attribute on a Vec<T>?

pub struct Packet {
    #[deku(bytes = "1")]
    length: u8,
    // Read all required bytes, then parse it.
    #[deku(bytes = "length - 2")]
    messages: Vec<Message>,
}

@wcampbell0x2a
Copy link
Collaborator Author

Would something like this make sense? Allowing the use of bytes/bits attribute on a Vec<T>?

pub struct Packet {
    #[deku(bytes = "1")]
    length: u8,
    // Read all required bytes, then parse it.
    #[deku(bytes = "length - 2")]
    messages: Vec<Message>,
}

lgtm

@sharksforarms
Copy link
Owner

I wish we could re-purpose bytes, but it looks like it's going to be complicated to implement. Currently bytes/bits are considered a Field context which would get passed to Message. This could be made easier with trait specialization. Unless @constfold knows a better way, we will need to introduce a new keyword to do this.

The easiest way for now is to create something like @constfold described above or use custom reader/writer attributes

@constfold
Copy link
Contributor

constfold commented Aug 1, 2020

Does impl<Ctx> DekuRead<(BitSize, Ctx)> for Vec<T>(or ByteSize) work? @sharksforarms
Ah, it doesn't when inner type need bytes too.
Seems like we must add a new attribute. But I prefer the idea commented in #76 (comment)
Ah, we can resuse count: #[deku(count = "BitSize(123)")]. or #[deku(ctx = "BitSize(123), (Whatever, ...)")]

@sharksforarms
Copy link
Owner

sharksforarms commented Aug 1, 2020

Does impl<Ctx> DekuRead<(BitSize, Ctx)> for Vec<T>(or ByteSize) work?

The DekuRead impls are ok, the DekuWrite impls get a bit funky because the bitsize is passed as a ctx instead of being used by Vec in the case of DekuRead (like <(Count, Ctx)>), DekuWrite is <Ctx>

Ah, we can resuse count: #[deku(count = "BitSize(123)")]

This is interesting....
Maybe rename to size? or len?
#[deku(size= "BitSize(len)")]
#[deku(size= "Count(len)")]

@sharksforarms
Copy link
Owner

With @samuelsleight changes, something like this may now be possible:

#[deku(bits = "5")]
field_a: Vec<u8>
#[deku(bytes = "3")]
field_a: Vec<u8>
impl<T: DekuRead> DekuRead<BitSize> for Vec<T>
{
    fn read(
        input: &BitSlice<Msb0, u8>,
        bit_size: BitSize,
    ) -> Result<(&BitSlice<Msb0, u8>, Self), DekuError>
    where
        Self: Sized
    {
        read_vec_with_predicate(input, Vec::new(), (), move |read_bits, _| {
            read_bits == bit_size.into()
        })
    }
}

@samuelsleight
Copy link
Contributor

So, I've just sent a PR (#124) that will allow the following code to work as expected:

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
pub struct Packet {
    length: u8,

    #[deku(bytes_read = "length - 2")]
    messages: Vec<Message>,
}

As no decision had yet been made here as to the exact attribute I've opted to make a decision just to get something functional implemented, and have gone with bytes_read and bits_read to avoid overloading any existing attributes - I welcome any better ideas should we come up with any however

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants