-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement load_buffer/check_buffer in RBinPlugins #13353
Conversation
bdffd38
to
7ef1752
Compare
Note that |
cool |
I'd say let's start by reviewing/merging this if travis is green. I'll do the rest of the plugins soon. |
sgtm
… On 12 Mar 2019, at 11:15, Riccardo Schirone ***@***.***> wrote:
I'd say let's start by reviewing/merging this if travis is green. I'll do the rest of the plugins soon.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#13353 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-lljR2EtdXcHqgtolw2uTHRDi7_nQks5vV34ugaJpZM4bpkWR>.
|
Also, I'm rewriting the plugins to just call Using r_buf_ref:
Using r_buf_new_with_bufref:
|
yes, that was expected to happen too. copying buffers is not necessary
… On 12 Mar 2019, at 11:21, Riccardo Schirone ***@***.***> wrote:
Also, I'm rewriting the plugins to just call r_buf_ref on the buffer passed to load_buffer. Does that sound like a problem to you? This way, we don't even create a new RBuffer, but we just use the one passed to load_buffer. The alternative may be to use a r_buf_new_with_bufref, which creates a new buffer structure but reuses the same data as the original buffer. What do you think?
Using r_buf_ref:
pros:
no additional memory used, at all
cons:
one single "buffer state", so if something changes the buffer state outside of the binplugin, it may affect the bin plugin as well, because the state is the same
Using r_buf_new_with_bufref:
pros:
no additional memory for the data
separate buffer state for the bin plugin (e.g. seek, etc.)
cons:
a little (very little, i'd say) of extra memory for the separate buffer state
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#13353 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-luHHgF0Mbr8B95QXrwUThUT0n-ziks5vV3-UgaJpZM4bpkWR>.
|
|
||
ut8 tmp[16]; | ||
int read_length = r_buf_read_at (buf, 0, tmp, sizeof (tmp)); | ||
if (read_length <= 0) { |
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.
what about != 16
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.
well != sizeof(tmp).
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'm trying to keep the original semantic. And originally the check was length > 0
, it wasn't really a requirement that 16 bytes were read.
Codecov Report
@@ Coverage Diff @@
## master #13353 +/- ##
==========================================
+ Coverage 36.88% 36.89% +<.01%
==========================================
Files 931 931
Lines 299060 299089 +29
==========================================
+ Hits 110321 110358 +37
+ Misses 188739 188731 -8
Continue to review full report at Codecov.
|
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.
.
7ef1752
to
55a42ef
Compare
Ok let's wait for Travis. I think if green it can be merged. |
Some of the bin plugins in #13349