Change Strings in SIP crate to PathBufs/OsStrings #2490
Conversation
gememma
force-pushed
the
sip-crate-strings
branch
3 times, most recently
from
1433be1
to
5d94b04
Compare
gememma
force-pushed
the
sip-crate-strings
branch
2 times, most recently
from
d057886
to
677f1d2
Compare
gememma
force-pushed
the
sip-crate-strings
branch
from
5bbce18
to
51a80ea
Compare
mirrord/layer/src/file/hooks.rs
Outdated
| @@ -57,7 +57,9 @@ fn update_ptr_from_bypass(ptr: *const c_char, bypass: Bypass) -> *const c_char { | |||
| // inside mirrord's temp bin dir. The detour has returned us the original path of the file | |||
| // (stripped mirrord's dir path), so now we carry out the operation locally, on the stripped | |||
| // path. | |||
| Bypass::FileOperationInMirrordBinTempDir(stripped_ptr) => stripped_ptr, | |||
| Bypass::FileOperationInMirrordBinTempDir(path_buf) => { | |||
| path_buf.as_os_str().as_encoded_bytes().as_ptr() as _ | |||
There was a problem hiding this comment.
Returning ptr into a PathBuf that goes out of scope here, not good.
mirrord/layer/src/detour.rs
Outdated
| FileOperationInMirrordBinTempDir(*const c_char), | ||
| FileOperationInMirrordBinTempDir(PathBuf), |
There was a problem hiding this comment.
Why do you want to return a new PathBuf here?
Why not the new pointer like before, or just the length of the removed prefix?
There was a problem hiding this comment.
We can't return a pointer into the original String because the switch over to using PathBuf means that operations like stripping a prefix result in a new PathBuf, where before they kept the same String buffer - so the FileOperationInMirrordBinTempDir needs to own the value, otherwise the pointer will be dangling
There was a problem hiding this comment.
I disagree. You don't need to return a PathBuf, as all the information the callers (that handle that variant) ever need, is how many bytes of the path to skip (this is equivalent to a new pointer into the original path buffer, either one works). If we still want to create new PathBufs (which I don't think is necessary) while finding out that number, we can still at the end just check the length difference of the old and new PathBufs, and only return that number.
mirrord/layer/src/exec_utils.rs
Outdated
| if let Bypass(FileOperationInMirrordBinTempDir(later_ptr)) = path_buf_detour { | ||
| if let Bypass(FileOperationInMirrordBinTempDir(path_buf)) = path_buf_detour { | ||
| let later_ptr: *const i8 = path_buf.as_os_str().as_encoded_bytes().as_ptr() as _; |
There was a problem hiding this comment.
If you change what later_ptr is, you also have to change the code that uses that variable in line 292 and after that, because it does not hold the information it used to hold anymore.
The calculation of prefix_len is now wrong. Making the prefix_len correct would be enough to make the rest of the function correct.
prefix_len is supposed to hold the length of the prefix that should be removed from the path.
It is currently calculated as
let prefix_len = later_ptr.offset_from(path);But after the change later_ptr and path are pointers into two different buffers, so taking their offset makes no sense.
If you really want FileOperationInMirrordBinTempDir to hold a PathBuf (which doesn't look convenient to me), then you could calculate prefix_len by reducing the length of path_buf from *buflen.
However I think it would be nicer to either leave the value of that variant a pointer as it was, or hold the prefix length directly (but then you would have to calculate the new pointer in the places in the code where the returned pointer was used as-is).
There was a problem hiding this comment.
I think calculating the length in a new way should be sufficient - I don't believe you can have FileOperationInMirrordBinTempDir contain a meaningful pointer with these changes
gememma
force-pushed
the
sip-crate-strings
branch
2 times, most recently
from
d39cb61
to
5feabae
Compare
gememma
force-pushed
the
sip-crate-strings
branch
from
5feabae
to
ec2b9f2
Compare
gememma
force-pushed
the
sip-crate-strings
branch
from
40a7dde
to
9710a74
Compare
|
The issue is in the backlog, closing this PR for now |
Closes #2198
This results in some lossy conversions where the SIP code is called from other crates which could be eliminated if all the Strings in the whole codebase were evaluated - there are plenty that would make more sense as OsStrings or Paths - but that would be a much larger issue. Eventually I think we could probably eliminate all the to_string_lossy() calls and avoid conversion to String altogether.