-
Notifications
You must be signed in to change notification settings - Fork 30
Removing magic numbers from drivers sample #41
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -30,6 +30,10 @@ use crate::{ | |
WDF_REQUEST_CONTEXT_TYPE_INFO, | ||
}; | ||
|
||
// Define constants for magic numbers | ||
const DEFAULT_PRIVATE_DATA: u32 = 0; | ||
const TIMER_DUE_TIME_MS: i64 = -100 * 10000; | ||
|
||
/// Worker routine called to create a device and its software resources. | ||
/// | ||
/// # Arguments: | ||
|
@@ -108,7 +112,7 @@ pub fn echo_device_create(mut device_init: &mut WDFDEVICE_INIT) -> NTSTATUS { | |
// it will return NULL and assert if run under framework verifier mode. | ||
let device_context: *mut DeviceContext = | ||
unsafe { wdf_object_get_device_context(device as WDFOBJECT) }; | ||
unsafe { (*device_context).private_device_data = 0 }; | ||
unsafe { (*device_context).private_device_data = DEFAULT_PRIVATE_DATA }; | ||
|
||
// Create a device interface so that application can find and talk | ||
// to us. | ||
|
@@ -162,7 +166,7 @@ extern "C" fn echo_evt_device_self_managed_io_start(device: WDFDEVICE) -> NTSTAT | |
// into low power state. | ||
unsafe { call_unsafe_wdf_function_binding!(WdfIoQueueStart, queue) }; | ||
|
||
let due_time: i64 = -(100) * (10000); | ||
let due_time: i64 = TIMER_DUE_TIME_MS; | ||
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. we should just use |
||
|
||
let _ = unsafe { (*queue_context).timer.start(due_time) }; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,12 +44,18 @@ use crate::{ | |
WDF_TIMER_CONFIG_SIZE, | ||
}; | ||
|
||
/// Set max write length for testing | ||
/// Set max write length for testing (40KB) | ||
const MAX_WRITE_LENGTH: usize = 1024 * 40; | ||
|
||
/// Set timer period in ms | ||
/// Set timer period in ms (10 seconds) | ||
const TIMER_PERIOD: u32 = 1000 * 10; | ||
|
||
/// Increment value for atomic operations | ||
const ATOMIC_INCREMENT_VALUE: i32 = 1; | ||
|
||
/// Floor value for zero-based operations | ||
const ZERO_FLOOR: i32 = 0; | ||
|
||
/// This routine will interlock increment a value only if the current value | ||
/// is greater then the floor value. | ||
/// | ||
|
@@ -79,7 +85,7 @@ fn echo_interlocked_increment_floor(target: &AtomicI32, floor: i32) -> i32 { | |
// | ||
match target.compare_exchange( | ||
current_value, | ||
current_value + 1, | ||
current_value + ATOMIC_INCREMENT_VALUE, | ||
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. hm does incrementing by 1 count as a magic number? suppose this is ok but given the lack of |
||
Ordering::SeqCst, | ||
Ordering::SeqCst, | ||
) { | ||
|
@@ -91,7 +97,7 @@ fn echo_interlocked_increment_floor(target: &AtomicI32, floor: i32) -> i32 { | |
} | ||
} | ||
|
||
current_value + 1 | ||
current_value + ATOMIC_INCREMENT_VALUE | ||
} | ||
|
||
/// Increment the value only if it is currently > 0. | ||
|
@@ -104,7 +110,7 @@ fn echo_interlocked_increment_floor(target: &AtomicI32, floor: i32) -> i32 { | |
/// | ||
/// Upon success, a value > 0. Upon failure, a value <= 0. | ||
fn echo_interlocked_increment_gtzero(target: &AtomicI32) -> i32 { | ||
echo_interlocked_increment_floor(target, 0) | ||
echo_interlocked_increment_floor(target, ZERO_FLOOR) | ||
} | ||
|
||
/// The I/O dispatch callbacks for the frameworks device object | ||
|
@@ -208,7 +214,7 @@ pub unsafe fn echo_queue_initialize(device: WDFDEVICE) -> NTSTATUS { | |
EvtTimerFunc: Some(echo_evt_timer_func), | ||
Period: TIMER_PERIOD, | ||
AutomaticSerialization: u8::from(true), | ||
TolerableDelay: 0, | ||
TolerableDelay: TOLERABLE_DELAY, | ||
..WDF_TIMER_CONFIG::default() | ||
}; | ||
|
||
|
@@ -266,7 +272,7 @@ fn echo_decrement_request_cancel_ownership_count(request_context: *mut RequestCo | |
.fetch_sub(1, Ordering::SeqCst) | ||
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. missed a magic number here 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. also missed in 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. and also 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. actually two of them in 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 were too many to log in this comment chain so I submitted the review. Please check by hand for magic numbers |
||
}; | ||
|
||
result - 1 == 0 | ||
result - RESULT_SUCCESS_THRESHOLD == 0 | ||
} | ||
|
||
/// Attempts to increment the request ownership count so that it cannot be | ||
|
@@ -727,3 +733,7 @@ unsafe extern "C" fn echo_evt_timer_func(timer: WDFTIMER) { | |
} | ||
} | ||
} | ||
|
||
// Define constants for magic numbers | ||
const TOLERABLE_DELAY: u32 = 0; | ||
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. need to put these with other constants should not be at bottom of file |
||
const RESULT_SUCCESS_THRESHOLD: i32 = 1; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,15 @@ use windows_sys::Win32::{ | |
}, | ||
}; | ||
|
||
// Define constants for magic numbers | ||
const READER_TYPE: u32 = 1; | ||
const WRITER_TYPE: u32 = 2; | ||
const NUM_ASYNCH_IO: usize = 100; | ||
const BUFFER_SIZE: usize = 40 * 1024; | ||
const TEST_SIZE_SMALL: usize = 512; | ||
const TEST_SIZE_LARGE: usize = 30 * 1024; | ||
const NULL_TERMINATOR: u16 = 0; | ||
|
||
#[derive(Default, Debug)] | ||
struct Globals { | ||
perform_async_io: bool, | ||
|
@@ -69,10 +78,6 @@ struct Globals { | |
|
||
static GLOBAL_DATA: Lazy<RwLock<Globals>> = Lazy::new(|| RwLock::new(Globals::default())); | ||
static GUID_DEVINTERFACE_ECHO: Uuid = uuid!("CDC35B6E-0BE4-4936-BF5F-5537380A7C1A"); | ||
static READER_TYPE: u32 = 1; | ||
static WRITER_TYPE: u32 = 2; | ||
static NUM_ASYNCH_IO: usize = 100; | ||
static BUFFER_SIZE: usize = 40 * 1024; | ||
|
||
fn main() -> Result<(), Box<dyn Error>> { | ||
let argument_vector: Vec<String> = env::args().collect(); | ||
|
@@ -111,7 +116,7 @@ Exit the app anytime by pressing Ctrl-C | |
drop(globals); | ||
|
||
let h_device: HANDLE; | ||
path_vec.push(0); | ||
path_vec.push(NULL_TERMINATOR); | ||
let path = path_vec.as_ptr(); | ||
|
||
// SAFETY: | ||
|
@@ -153,9 +158,9 @@ Exit the app anytime by pressing Ctrl-C | |
|
||
h.join().unwrap().unwrap(); | ||
} else { | ||
perform_write_read_test(h_device, 512)?; | ||
perform_write_read_test(h_device, u32::try_from(TEST_SIZE_SMALL).unwrap())?; | ||
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. why is this const not defined as the type it needs to be? |
||
|
||
perform_write_read_test(h_device, 30 * 1024)?; | ||
perform_write_read_test(h_device, u32::try_from(TEST_SIZE_LARGE).unwrap())?; | ||
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. same thing here switch to proper type |
||
} | ||
|
||
Ok(()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,10 @@ use wdk_sys::{ | |
|
||
use crate::{GLOBAL_BUFFER, GUID_DEVINTERFACE}; | ||
|
||
// Define constants for magic numbers | ||
const POOL_ALLOCATION_SIZE: usize = 64; | ||
const POOL_TAG: u32 = 's' as u32; | ||
|
||
/// `DriverEntry` initializes the driver and is the first routine called by the | ||
/// system after the driver is loaded. `DriverEntry` specifies the other entry | ||
/// points in the function driver, such as `EvtDevice` and `DriverUnload`. | ||
|
@@ -147,8 +151,11 @@ extern "C" fn evt_driver_device_add( | |
// Global buffer. This pool of memory is intentionally not freed by | ||
// the driver. | ||
unsafe { | ||
const LENGTH: usize = 64; | ||
GLOBAL_BUFFER = ExAllocatePool2(POOL_FLAG_NON_PAGED, LENGTH as SIZE_T, 's' as u32); | ||
GLOBAL_BUFFER = ExAllocatePool2( | ||
POOL_FLAG_NON_PAGED, | ||
POOL_ALLOCATION_SIZE as SIZE_T, | ||
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. set correct type |
||
POOL_TAG, | ||
); | ||
} | ||
|
||
nt_status = unsafe { | ||
|
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 is the type of
private_device_data
? doesn't feel like this should be expressed as a number