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

IOWrite using TryIntoCtx #36

Open
roblabla opened this issue Sep 14, 2018 · 3 comments
Open

IOWrite using TryIntoCtx #36

roblabla opened this issue Sep 14, 2018 · 3 comments

Comments

@roblabla
Copy link

roblabla commented Sep 14, 2018

Lots of types implement TryIntoCtx but not IntoCtx in goblin (like elf::ProgramHeader). It'd be nice if those types would be usable with IOWrite. Furthermore, this would grant the user more control over error handling.

Maybe something like:

fn iowrite_with_try<N>(&mut self, n: N, ctx: Ctx) -> Result<(), N::Error>
where
    N: scroll::ctx::SizeWith<Ctx, Units = usize> + scroll::ctx::TryIntoCtx<Ctx>>
    N::Error: From<std::io::Error>
{
    let mut buf = [0u8; 256];
    let size = N::size_with(&ctx);
    let buf = &mut buf[0..size];
    n.try_into_ctx(buf, ctx)?;
    self.write_all(buf)?;
    Ok(())
}
@m4b
Copy link
Owner

m4b commented Sep 22, 2018

Hi @roblabla thanks for opening the issue!

So:

  1. I don't like the function name per say, but I like the idea!
  2. It would also be nice to cleanup the iowrite stack array in (rare) cases where the element is larger than 250; we could perhaps heap allocate in that case, but it might be tricky, not sure (not required for this issue, but just mentioning it here, since it would probably copy the same stack buffer stuff.

But yea, I'll merge a PR with tests and examples for this, seems good to me; but will probably require some bikeshedding on a good name?

@roblabla
Copy link
Author

roblabla commented Sep 22, 2018

Yeah, the name is pretty bad, I didn't think long and hard on it :P.

For the stack array, there are crates like smallvec which provide stack arrays under a certain size, and heap arrays otherwise. We could move to something like (untested):

let mut buf = SmallVec::<[u8; 256]>::new();
let size = N::size_with(&ctx);
let buf = buf.resize(size, 0);
n.try_into_ctx(buf, ctx)?;
self.write_all(buf)?;
Ok(())

I'll try to write a PR soon:tm:

@m4b
Copy link
Owner

m4b commented Sep 22, 2018

Ok, but just fyi, I don't really want external deps in scroll; I know that's super annoying, but I'd like to keep it dependency free in general.

That being said I think your general design is good, and I'd even be ok wtih merging a 256 byte temporary struct if it gets this issue closed (as switching to heap allocation for objects larger than 256 bytes is somewhat orthogonal to your requirements). Anyway, thanks for your interest, and no rush :)

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

No branches or pull requests

2 participants