Closed mgeisler closed 4 months ago
Using a randomized temp directory should fix this.
Right after implementing this, I noticed https://github.com/rust-vmm/vmm-sys-util/blob/main/src/tempfile.rs :slightly_smiling_face: I don't really know the crate here, so would it be better to use the functionality there?
Hi! Thanks for this PR! I agree that the failure you're seeing it because of the hardcoded path in the test. Probably something else on the system created a
test
directory inside/tmp
, and so the test fails when it tries to createtest
as a file. (assuming that no one created something called "test" inside of/tmp
is indeed bold of this test, haha)
Yeah, it's the kind of hidden assumption that works fine for a long time, until it doesn't because someone has an unexpected setup :smile:
As for
tempfile
, let use the module from vmm-sys-util. We try to keep the dependencies on these crates minimal, so since its an easy change to use our module here, that'd be preferred.
Alright! I applies your suggestion to use TempFile
from this crate, but tweaked them a bit. I think using the temporary file directly does something slightly different, but I'm not sure if it matters? Before the test
file did not exist when open()
was called. If we use the temp_file
directly, the file exists — and I guess it's a bit of a coincidence that the flags match up?
To avoid reasoning about this, I now simply delete the temp_file
early and then reuse the path
(with the assumption that nobody is trying to race me).
Please let me know if you think this is overkill?
As for
tempfile
, let use the module from vmm-sys-util. We try to keep the dependencies on these crates minimal, so since its an easy change to use our module here, that'd be preferred.Alright! I applies your suggestion to use
TempFile
from this crate, but tweaked them a bit. I think using the temporary file directly does something slightly different, but I'm not sure if it matters? Before thetest
file did not exist whenopen()
was called. If we use thetemp_file
directly, the file exists — and I guess it's a bit of a coincidence that the flags match up?
I also don't think it really matter, as long as we do something that causes a well-known errno to be set (e.g. reading from a file opened with O_WRONLY
resulting in EBADF).
To avoid reasoning about this, I now simply delete the
temp_file
early and then reuse thepath
(with the assumption that nobody is trying to race me).Please let me know if you think this is overkill?
No, this is perfectly fine, thanks for fixing this!
Thanks, I'm glad you like it! The tests will then soon also be running as part of the Android build: https://r.android.com/3044134.
Summary of the PR
While trying to enable the unit tests in Android (see https://r.android.com/3044134), I noticed test failures saying:
I believe this is because of the hard-coded name for the temporary file in the
test_errno
unit test.Using a randomized temp directory should fix this.
Requirements
Before submitting your PR, please make sure you addressed the following requirements:
git commit -s
), and the commit message has max 60 characters for the summary and max 75 characters for each description line.unsafe
code is properly documented.