search

m4b / goblin

An impish, cross-platform binary parsing crate, written in Rust
MIT License
1.17k stars 156 forks source link

Fix corrupted file_alignment #332

Closed anfedotoff closed 1 year ago

anfedotoff commented 1 year ago

Fix for #331

m4b commented 1 year ago

@koutheir does this look ok to you?

koutheir commented 1 year ago

Thank you for your efforts, @anfedotoff

m4b commented 1 year ago

Some notes:

  1. We won't be adding anyhow/thiserror/error library to goblin, probably ever. So if the assumption of these result returning changes is they'll be more usable in a follow up PR that uses more context via anyhow/whatever, want to be clear this is not a good assumption to make :) I want to keep the dependencies as thin as possible.
  2. i think the signature of find_offset returning Option was fine, and more flexible (allowing users to convert that into a more specific error if need be), for a utils library function (that maybe/probably shouldn't be exposed, but it is useful). I'm 50/50 on the result signature, because that forces our error type onto user (and technically a string allocation) that uses this function directly. I also don't have any data on if anyone is even using this function so also maybe doesn't matter that much. I'm not convinced though that semantically it's an error returning function (just like HashMap::get doesn't return an error, or various find methods in stdlib generally don't return error).

So I'm wondering why couldn't all the places find_offset was used with the new Result signature not have used the function find_offset_or or left it with ok_or semantics? This is probably annoying question since all the work was done to make it use the result form now, but just curious :) I'm just wondering if this is not the best breaking change, specifically:

  1. It is an avoidable break as far as I can see
  2. It doesn't really address some necessary change like a field going from T -> Option
  3. It doesn't add much in the way of future proofing (this is also similar to 1., in that the user can just convert the option to error if they want)

Anyway, I think it's probably fine if this ends up being better maintainable code, but it is a breaking change, so it'll get merged in a little later (along with the other breaking change that's been pending).

On that note, may be a good time to think of any other breaking changes for PE stuff that would be good to get in (and it's fine if there are none!)

m4b commented 1 year ago

@anfedotoff i'd be ok with merging this but i think we have to roll back the breaking change to the find_offset function; it seems unnecessary to me. If you want a result in the callsites, I'd suggest using find_offset_or and restore the error messages. Then we can merge and it won't be breaking, otherwise I think this PR will just languish.

Thanks for your patience!

anfedotoff commented 1 year ago

@anfedotoff i'd be ok with merging this but i think we have to roll back the breaking change to the find_offset function; it seems unnecessary to me. If you want a result in the callsites, I'd suggest using find_offset_or and restore the error messages. Then we can merge and it won't be breaking, otherwise I think this PR will just languish.

Thanks for your patience!

Sorry, for late reply. It's ok if you don't want breaking changes. If we don't want to use anyhow crate, then, from my point of view, all changes have no sense in this pull request. I think, better is to roll back to one line fix, before calling function with bad arguments. Or we could find some better place to insert this one if. What do you think?

m4b commented 1 year ago

@anfedotoff Ok that works for me!

anfedotoff commented 1 year ago

@anfedotoff Ok that works for me!

I've done something strange, playing with git history and doing rebase master. I'll better reopen new PR from fresh master. Fix will be very short, sorry)