Feature/zcopy dser

[alambert-lanl]

The flag to enable this is --zero-copy

[bertschinger]

this change should be pulled out into a separate PR and merged in first, since it's a bug fix of a pre-existing bug that doesn't depend on the rest of the code here

[bertschinger]

I think this needs to be broken up into smaller PRs, it's a little too large to review all at once.

Can you start with a PR that only handles non-recursive structs? Unions, linked lists, can just be skipped, you can just throw a todo!() on them for now.

I want to focus on getting the simplest cases right first and that'll be easier with a much more focused PR.

[bertschinger]

I would probably pull all of the new zcopy functions into a new file, somethng like deserialize_zc.rs to help keep things organized

[bertschinger]

It's pretty hard to read these byte vectors. In general I'd rather define a Rust struct that contains the data, use the allocating serialization APIs to convert it into an array of bytes, and then test the zcopy functions on that byte array.

Admittedly that doesn't work for testing error cases where we want to ensure that trying to read an invalid message returns an error. For those cases, you can either hand-write the byte vector, or serialize the rust struct into a byte vector which you then modify (for example by overwriting the length of an array with a too long length or something).

[bertschinger]

This flag should probably be specific to deserialization - if a zero copy serialization API is later added, that can be a separate flag

[bertschinger]

Enabling zero copy shouldn't prevent compiling the allocating (or non-allocating) routines. Right now, this is problematic because compiling the zero-copy deserialize routines doesn't produce any serialization code so there's no way to use this in a real app that needs to do both sides.

It should be possible to generate:

• just alloc or just no-alloc
• zcopy + alloc
• zcopy + no-alloc

[bertschinger]

I see that the iterator has an err field - I guess this is meant to indicate that the message was corrupt and the user of this API is expected to check that field after None is returned to see whether iteration finished normally or abnormally.

That API seems a bit easy to misuse. Will users of the API know that they have to check iter.err?

Maybe the type should be xdr_lib::Result<T> and when the message is found to be invalid, Some(Err(_)) is returned. This is still a bit inelegant because then one more next() call will be required to return None. Unfortunately fallible iterators seem a bit clunky but making it return Result<T> seems harder to misuse by forgetting to check that a message was corrupt.

[bertschinger]

These signatures should return the less verbose xdr_lib::Result<Self>

[bertschinger]

I don't think users of the API should have to import this trait. There should be a constructor that doesn't depend on a trait. It can always just call from_buf() if that's the implementation that makes the most sense:

    impl<'a> FooReader<'a> {
          pub fn new(buf: &'a [u8]) -> xdr_lib::Result<Self> {
              Self::from_buf(buf)
          }
    ....

[bertschinger]

most of these members don't seem like they should be public

[bertschinger]

I think the OnceCell pattern makes this code trickier and more complex than it needs to be.

From looking at a simple struct case:

struct foo {
    opaque str<>;
    int a;
};

it seems that the width is always filled in at construction time:

from_buf()
  -> validate()
    -> get_str_width() // sets width

later, the width is read in

get_a()
  -> get_str_width() // gets width

Soget_str_width() is effectively used as both a getter and a setter.

I would rather the struct be defined as

    pub struct fooReader <'a> {
        buf: &'a [u8],
        str_width: usize,
    }

And the constructor could be something like:

        fn from_buf(buf: &'a [u8]) -> xdr_lib::Result<Self> {
            let str_width = Self::initialize_str_width(buf)?;
            let me = Self {
                buf,
                str_width,
            };
            me.validate()
        }

then later, instead of calling self.get_str_width(), simply access the field directly:

        pub fn get_a(&self) -> i32 {
            xdr_lib::get_i32_immut(&self.buf[self.str_width])
        }

Sure, the OnceCell pattern prevents redefining the value once it's initialized - but actually, it's not possible to do that anyways because there are no methods on fooReader that take a mutable reference. That makes fooReader effectively an immutable object, so the OnceCell isn't providing a useful service here.

[alambert-lanl]

closing in favor of #21