-
Notifications
You must be signed in to change notification settings - Fork 2
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
Initial code review #1
Changes from 2 commits
ed5f36a
7219dcc
191f94f
1f56606
f20e220
f36e994
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,46 +171,29 @@ impl AssertionInfo { | |
|
||
// Verify that the TrackingInfo for self in | ||
// ASSERT_TRACKER has been updated according to self.condition | ||
fn track_entry(&self, tracker: &mut HashMap<String, TrackingInfo>) { | ||
|
||
fn track_entry(&self) { | ||
// Requirement: Catalog entries must always will emit() | ||
if !self.hit { | ||
self.emit(); | ||
return | ||
} | ||
|
||
let tracking_key = self.id.clone(); | ||
|
||
// Establish TrackingInfo for this trackingKey when needed | ||
let maybe_info = tracker.get(&tracking_key); | ||
if maybe_info.is_none() { | ||
tracker.insert(self.id.clone(), TrackingInfo::new()); | ||
} | ||
|
||
// Record the condition in the associated TrackingInfo entry | ||
let condition = self.condition; | ||
let tracked_entry = tracker.entry(self.id.clone()); | ||
tracked_entry.and_modify(|e: &mut TrackingInfo| { | ||
if condition { | ||
e.pass_count += 1; | ||
} else { | ||
e.fail_count += 1; | ||
} | ||
}); | ||
|
||
// Really emit the assertion when first seeing a condition | ||
if let Some(tracking_info) = tracker.get(&tracking_key) { | ||
let pass_count = tracking_info.pass_count; | ||
let fail_count = tracking_info.fail_count; | ||
if condition { | ||
if pass_count == 1 { | ||
self.emit() | ||
} | ||
} else if fail_count == 1 { | ||
self.emit() | ||
} | ||
let mut tracker = ASSERT_TRACKER.lock().unwrap(); | ||
let info = tracker.entry(self.id.clone()).or_default(); | ||
// Record the condition in the associated TrackingInfo entry, | ||
// and emit the assertion when first seeing a condition | ||
let emitting = if self.condition { | ||
info.pass_count += 1; | ||
info.pass_count == 1 | ||
} else { | ||
info.fail_count += 1; | ||
info.fail_count == 1 | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps comment out the setting of |
||
|
||
drop(tracker); // release the lock asap | ||
if emitting { | ||
self.emit(); | ||
} | ||
Comment on lines
+183
to
+197
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minimize the critical section in which we hold the lock to |
||
} | ||
|
||
fn emit(&self) { | ||
|
@@ -260,7 +243,7 @@ pub fn assert_impl( | |
|
||
|
||
let assertion = AssertionInfo::new(assert_type, display_type, condition, message, class, function, file, begin_line, begin_column, hit, must_hit, id, details); | ||
let _ = &assertion.track_entry(ASSERT_TRACKER.lock().as_deref_mut().unwrap()); | ||
let _ = &assertion.track_entry(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to ignore the return value from .track_entry() - there is no longer a return value defined |
||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -617,7 +600,7 @@ mod tests { | |
let mut tracking_data = TrackingInfo::new(); | ||
|
||
let tracking_key: String = key.to_owned(); | ||
match ASSERT_TRACKER.lock().as_deref().unwrap().get(&tracking_key) { | ||
match ASSERT_TRACKER.lock().unwrap().get(&tracking_key) { | ||
None => tracking_data, | ||
Some(ti) => { | ||
tracking_data.pass_count = ti.pass_count; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
use libc::c_char; | ||
use libc::{c_char, size_t}; | ||
use libloading::{Library, Symbol}; | ||
use serde_json::{Value}; | ||
use std::ffi::{CString}; | ||
use std::io::{Error}; | ||
use std::sync::{Once, Mutex, Arc}; | ||
|
||
|
@@ -44,10 +43,10 @@ impl VoidstarHandler { | |
|
||
impl LibHandler for VoidstarHandler { | ||
fn output(&mut self, value: &Value) -> Result<(), Error> { | ||
let payload = value.to_string(); | ||
unsafe { | ||
let json_data_func: Symbol<unsafe extern fn(s: *const c_char)> = self.voidstar_lib.get(b"fuzz_json_data").unwrap(); | ||
let payload = CString::new(value.to_string())?; | ||
json_data_func(payload.as_ptr()); | ||
let json_data_func: Symbol<unsafe extern fn(s: *const c_char, l: size_t)> = self.voidstar_lib.get(b"fuzz_json_data").unwrap(); | ||
json_data_func(payload.as_bytes().as_ptr() as *const c_char, payload.len()); | ||
} | ||
Ok(()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
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.
Uses the entry API to simplify constructing a default
TrackingInfo
for a vacant tracking key.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.
With the access to the ASSERT_TRACKER contained (is it?) entirely here, consider relocating ASSERT_TRACKER to this function.