-
Notifications
You must be signed in to change notification settings - Fork 155
disk_striped: Fix the divide-by-zero error when chunk_size_in_kb field is missing in VTL2 settings #2153
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?
Conversation
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.
Pull Request Overview
Fixes a divide-by-zero panic in the disk striping functionality when the chunk_size_in_kb field is missing from VTL2 settings protobuf schema. The issue occurred because missing protobuf fields default to 0, which caused division by zero in subsequent calculations.
- Adds a filter to check for zero chunk size values before using the default
- Prevents divide-by-zero errors when chunk_size_in_kb is missing from protobuf
let read_only = devices[0].is_read_only(); | ||
let chunk_size_in_bytes = chunk_size_in_bytes.unwrap_or(CHUNK_SIZE_128K); | ||
let chunk_size_in_bytes = chunk_size_in_bytes | ||
.filter(|&chunk_size| chunk_size != 0) | ||
.unwrap_or(CHUNK_SIZE_128K); | ||
if !chunk_size_in_bytes.is_multiple_of(sector_size) { | ||
return Err(NewDeviceError::InvalidChunkSize( | ||
chunk_size_in_bytes, | ||
sector_size, | ||
)); | ||
} |
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.
Elegant fix, but given that this code is already doing some validation, suggest you actually just return an error to the user. Or, are there some cases where you think this would regress something?
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.
TFTR Matt!
There are two scenarios where chunk_size_in_bytes
will be Some(0)
(1) when it's not explicitly specified in the VTL2 settings - This is possible as per the bug
(2) set to 0 by the user.
Now for the latter scenario, we could return an error but for the former, IMO, it would break the existing user experience and tests probably (tests should be okay as we could fix them up).
But the intention of using unwrap_or()
on chunk_size_in_bytes
field was to set it to default when it's not specified, hence followed on the similar lines to accommodate the fix.
Please let me know your thoughts too :)
If this is a case where a field is missing why are we deserializing it into Some(0) instead of None? |
Issue:
The VTL2 settings processing code crashes with a divide-by-zero panic when the chunk_size_in_kb field is missing from LUN configurations in the protobuf schema.
Closes: #1796
Fix:
Fix the code wherein
chunk_size_in_bytes
takes the default value when the field is missing in the protobuf schema.From the striped code,
Include a conditional filter for setting the chunk_size to default(
CHUNK_SIZE_128K
) for such scenarios.Validation:
Locally ran vmm_tests(striped_disk related tests) successfully.