Conversation
- Add bounds checking using error_string_max_size before writing error strings - Fix incorrect character replacement: use '\0' (nul byte) instead of '0' (digit) Signed-off-by: Stephan Hügel <shugel@tcd.ie>
This should speed up grid operations that require multiple network requests.
| NonNull::new(header).expect("Failed to create non-Null pointer when building header value"), | ||
| ); | ||
| Ok(header) | ||
| network_client |
There was a problem hiding this comment.
Rather than storing Strings and converting here, I updated the network request logic to persist the CStrings, which makes this code a little simper.
| Err(e) => { | ||
| // The assumption here is that if 0 is returned, whatever error is in out_error_string is displayed by libproj | ||
| // since this isn't a conversion using CString, nul chars must be manually stripped | ||
| let err_string = e.to_string().replace('0', "nought"); |
There was a problem hiding this comment.
Don't miss this! I got rid of the \0 handling. We should probably figure it out, but maybe better to do so in the context of #243 first, and then I can rebase.
| /// Perform an HTTP range request starting at `offset` for `output_buffer.len()` bytes. | ||
| /// The response body is written into `output_buffer` and the response headers are | ||
| /// cached in `most_recent_response_headers`. Returns the number of bytes read. | ||
| fn read(&mut self, offset: u64, output_buffer: &mut [u8]) -> Result<usize, ProjError> { |
There was a problem hiding this comment.
This read method DRYs up network_open and network_read a bit.
| mut make_request: impl FnMut() -> Result<http::Response<ureq::Body>, ProjError>, | ||
| ) -> Result<http::Response<ureq::Body>, ProjError> { | ||
| let mut retries = 0; | ||
| let mut res = make_request()?; |
There was a problem hiding this comment.
Slightly tweaked this method - it now makes the initial request as well as potential retry requests. I think it flows a little better.
| unsafe { std::slice::from_raw_parts_mut(buffer.cast::<u8>(), size_to_read) }; | ||
|
|
||
| match unsafe { _string(url) } | ||
| .map_err(ProjError::from) |
There was a problem hiding this comment.
Error handling in this method is a little clunky. The facade pattern was nicer for this, but I estimated it wasn't worth it for these 3 lines of error handling.
| Ok(network_client) => { | ||
| // clear out any error message when successful | ||
| unsafe { | ||
| *out_error_string = 0; |
There was a problem hiding this comment.
I think this is equivalent to the previous:
let err_string = "";
unsafe {
out_error_string.copy_from_nonoverlapping(err_string.as_ptr().cast(), err_string.len());
out_error_string.add(err_string.len()).write(0);
}
2 participants
CHANGES.mdorCHANGELOG.mdBased on #243 (though I rebased it), so merge that first.
Fixes #206