rust-bakery / cookie-factory Goto Github PK

Now that we accept any write, if we do something like pair(foo, bar) and hit e.g WouldBlock after writing foo, we are in a state where we already wrote foo, but not yet bar, and reusing the same combinator would write foo a second time.

This PR moved some files around, including the license file, to a location that is rather unusual for Rust projects:
#38

The package.include setting in Cargo.toml wasn't updated for the new file location of the license text.

I'm not sure why "cargo publish" doesn't at least print a warning if explicitly included files aren't present :(

Currently SerializeFn is always returning the input and the written length. With the change to work on everything that implements Write, this does not seem necessary anymore and complicates code more than needed.

• Write already keeps track of the current position and we pass a &mut Write around anyway, so there's no point in returning the current input
• For counting the written length, if actually needed, one could use the Write directly or we could add a convenience wrapper around any Write that simply counts how much was written and returns that value on request. As a side-effect this makes the chain combinator unneeded and allows to use ? directly, and allows to not calculate the length if not needed. In addition to the plain Write wrapper, we could also have a length() function again that calculates the length.

@Keruspe @Geal Any opinions? I'd be happy to work on changing this if you agree.

Hi All,

I've been using this library, and it's fantastic. However I've noticed the no-std feature only works on the current code in git, not what's published on crates.io.

Is it possible to publish a release, or even pre-released version to crates.io?. That way I can depend on it in my crate.

It looks like there is a single commit added after the last publish that fixes the issue I am getting too 3adb3c3

I understand that

let buf = foo(buf)?;
let buf = bar(buf)?;
Ok(buf)

can feel smoother than

let (buf, len) = foo(buf)?;
let (buf, len) = bar(buf).map(|(buf, len2)| (buf, len + len2))?;
Ok((buf, len))

Though I still think this is an acceptable compromise:

let gen = foo.chain(&bar);
gen(buf)

One of the point that was brought up was the need to calculate the written length. In all the provided helpers, the write trait already gives us this data. Is there any concrete example of a case where this would be expansive?

Having to wrap everything in WriteCounter, and introducing hacks such as BackToTheBuffer, when used in real world applications, is really expansive on the other hand.

Take this simple integration test for example: https://github.com/sozu-proxy/amq-protocol/blob/b2db9c71bc617424cfe018ad9cd6c0f13273ec7e/types/tests/integration.rs
It uses BackToTheBuffer and WriteCounter recursively to write stuff (imagine a table of elements, which can themselves be tables, etc).

I had to add #![recursion_limit="1024"] for it to even start to compile (compiler suggested 128, then 256, etc..).
Once recursion limit has been set comes the type length, I did tenth of compiler cycles by now I think, and still haven't reached a value for which it actually finishes building. Current value is #![type_length_limit="1630195223"]

As the title says, back_to_the_buffer can not easily be used from within another back_to_the_buffer call, which makes it hard to serialize some nested formats.

The default/recommended way to use back_to_the_buffer would be to use gen(<serializer>) inside the gen closure like in the docs.

back_to_the_buffer(
    4,
    move |buf| gen(string("test"), buf), // < this line
    move |buf, len| gen_simple(be_u32(len as u32), buf)
)

However, this wraps buf in a second WriteContext, so that the serializer is called with a WriteContext<WriteContext<W: Write + BackToTheBuffer>>.
With this, we can no longer call back_to_the_buffer again, since it requires a WriteContext<W: Write + BackToTheBuffer>.

It took me quite a while to come up with this solution: (adapted to the doc example)

back_to_the_buffer(
    4,
    move |buf| {
        let begin = buf.position;
        (string("test")(buf)).map(|wctx| {
            let pos = wctx.position;
            (wctx, pos - begin)
        })
    },
    move |buf, len| gen_simple(be_u32(len as u32), buf)
)

Now back_to_the_buffer could be called again in the stringcombinator.

Could this be fixed/made easier in a nice way?
If not, maybe this example could at least be added to the docs so that others don't run into the same wall as I did.

Thanks!

$ rustc +nightly --version
rustc 1.43.0-nightly (07a34df18 2020-02-08)
$ cargo +nightly build
...
   Compiling cookie-factory v0.3.0
error[E0415]: identifier `s` is bound more than once in this parameter list
   --> /home/str4d/.cargo/registry/src/github.com-1ecc6299db9ec823/cookie-factory-0.3.0/src/internal.rs:144:36
    |
144 |     fn skip(s: WriteContext<Self>, s: usize) -> GenResult<Self>
    |                                    ^ used as parameter more than once

error: aborting due to previous error

For more information about this error, try `rustc --explain E0415`.
error: could not compile `cookie-factory`.
warning: build failed, waiting for other jobs to finish...
error: build failed

Builds fine with latest stable (1.41.0).

I'm not able to compare GenError. I'm writing a unit test for my serialized type and the following cannot compile:

        let mut s = [0; 23];
        let res = match foo.as_bytes((&mut s, 0)) {
            Ok(r) => r,
            Err(e) => assert_eq!(e, GenError::BufferTooSmall(1)),
        };

error[E0369]: binary operation `==` cannot be applied to type `cookie_factory::GenError`
   --> <redacted>
    |
114 |             Err(e) => assert_eq!(e, GenError::BufferTooSmall(1)),
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                       |
    |                       cookie_factory::GenError
    |                       cookie_factory::GenError
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error; 9 warnings emitted

With the recent removal of length from the GenResult, I face an issue:

I used to have this code:

/// Trait used to skip part of a buffer to later write inside the skipped part
pub trait SkipBuffer<'a> {
    /// Skip part of a buffer to later write inside the skipped part
    fn skip_buffer(self, offset: usize) -> (&'a mut [u8], Self);
}

impl<'a> SkipBuffer<'a> for &'a mut [u8] {
    fn skip_buffer(self, offset: usize) -> (&'a mut [u8], Self) {
        self.split_at_mut(offset)
    }
}

/// Apply a generator and serialize its length at the beginning of buffer
pub fn gen_with_len<'a, W: Write + SkipBuffer<'a>, F>(x: W, f: F) -> GenResult<W>
where
    F: Fn(W) -> GenResult<W>
{
    let (len_buf, x) = x.skip_buffer(4);
    let (x, len) = f(x)?;
    gen_long_uint(len_buf, len as LongUInt)?;
    Ok((x, len + 4))
}

Now, to get the length, I have to go through WriteCounter. Changing the signature of F from W to WriteCounter<W> should be enough, but then I need to implement SkipBuffer for WriteCounter<W> where W: SkipBuffer.

I currently see those solutions:

• add a constructor of WriteCounter that taked the Write and an offset
• change the Skip signature to return the skipped buffer too, and implement it for WriteCounter<W> where W: Skip
• merge SkipBuffer into cookie-factory

What do you think @Geal @sdroege ?

Opening issue to track the following:

Iterating on containers can be for in three different ways: using &v,
ref v, or v. Currently we cannot guess which notation to use, thus
provide three macros.
The solution in 822020d is unsatisfying, until we have a better one.

Currently all types are directly at the top-level of the crate. It potentially makes sense to move them into various sub-modules similar to how it's done in nom.

As part of that, the compat std::io module could also be moved into a nicer place.

And maybe we could also move the macros out of the way into some kind of legacy module and mark them as deprecated :)

What do you think?

@Geal what more do we want for making a 0.3 final release? #23 maybe?

I'd love getting a 0.3 soon to issue a amq-protocol 3.0 release with it

gen_le_i32!(x, -1)

fails with the following error:

error[E0600]: cannot apply unary operator `-` to type `u32`
  --> src/serialize.rs:13:32
   |
13 |         None => gen_le_i32!(x, -1),
   |                                ^^

Hello,
I'm looking into a library that will be capable to serialize structs representing some specified protocol, and I've learned about cookie-factory, and it looks super interesting!
I have one question regarding usage of cookie-factory: I have a protocol that have numerous single bits flags (similar to TCP frame), and on the Rust side it is represented by struct with bools. What is the best way to serialize this into bytes? I couldn't find anything in documentation.
Thanks!

If I try to write a u32 in a buffer of size 3, currently, I get BufferTooSmall(4), because that's the minimum size required by u32.
I we instead got BufferTooSmall(1), i.e. the missing space, it would make things easier for the caller to reallocate exactly what's needed