Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add MBR format #241
base: master
Are you sure you want to change the base?
Add MBR format #241
Changes from all commits
467ede9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was refactored a bit in master, is now
interp.RegisterFormat(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will rebase and fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do a bit field struct here (struct with
d.FieldBool("...")
) if it's useful to be able to address individual bits. Sometimes i also use0b1000_0000
etc literals for binary literalsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could use
scalar.UToSym
that way you can write queries as.boot_indicator == "active"
and if you really want to compare the non-mapped value do(.boot_indicator | toactual) == 0x80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe field for this byte also? otherwise it will just be skipped and end up as "unknown" fields. fq figures out where there is gaps and fill them in with unknown fields, so decoders should only skip things that are truely unknown. The idea is to always make all bits "addressable", is quite useful when digging into broken or strange files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes when i have problem figure how to model i think about how it will feel and look to user doing queries etc, ex for this a query to get partition size will be
.partition_table.entry_1.partition_size
for struct vs.partition_table[0].partition_size
for array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this to just
d.U2()
ord.U(2)
and it will do the error check and panic. I should probably renameBits
toTryBits
, the convention usually isTry*
functions returns error other just panic and there is usually pairs of functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh the cylinder bits are "continuous" as 16LE, this is a bit messy to represent :( i've run into this issue with some other formats like https://github.com/wader/fq/blob/master/format/vpx/vp8_frame.go#L36 haven't figured any nice way to handle this yet. Also would be nice if there was some decoding helpers for doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it make sense to read CHS as a struct with cylinder, head , sector fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and that suggestion is in line with this general tip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, forgot i wrote that :) maybe should add something more about that it also makes it easier to write queries if things are split up into more individual fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan is to use some kind of x86_16 decoder here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first iteration I'm going to focus on the partition table. Your work on the x86_16 decoder can be reused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the generated help text maybe this is redundant? I've used this to note missing/extra features etc