-
Couldn't load subscription status.
- Fork 293
CA-382640: open SHM with os.open to allow for RW/Creat #5405
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
Conversation
3ad8980 to
35bded8
Compare
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.
This way of opening the file allows for writing on random position without truncating it.
|
Since @bernhardkaindl 's PR (#5384) had merged master, could you rebase master and re-run the latest CI checkers? |
Signed-off-by: Mark Syms <mark.syms@citrix.com>
35bded8 to
5235115
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5405 +/- ##
=========================================
Coverage ? 45.38%
=========================================
Files ? 18
Lines ? 2937
Branches ? 0
=========================================
Hits ? 1333
Misses ? 1604
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…le otherwise): Add a comment based on Pau's explanation for the change in the PR: # Open file at pos 0, creating but not truncating the file(not possible otherwise): Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
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.
This manually created sequence of open(fdopen(file) appears like it works as advertised:
Test which shows how the change works:
echo "content" >file; # initial statustus
# optional check to test overwrite without truncating it:
python2 -c 'import os;os.fdopen(os.open("file", os.O_RDWR|os.O_CREAT),"r+b").write("CON")'
cat file
rm file
python2 -c 'import os;os.fdopen(os.open("file", os.O_RDWR|os.O_CREAT),"r+b").write("CON\n")'
cat fileThe output is as advertised, correct (verified on XS8 using XS8 Python2):
CONtent (from the 1st run, where "content" was the original content)
CON (from the 2nd run, where the file was deleted and it had to be created by `os.O_CREAT`.Based on Pau's explanation of the change, confirmed using the commands above:
This way of opening the file allows for writing on random position without truncating it.
Looks good, and could not find a better way to achieve it. 👍 (we should add a unit test....)
Done, and found that as usual the unit test successfully takes the "human factor" of error out of the loop - still many times, the bugs originate between the keyboard and the seat! ;-)
Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
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.
"Should be fine now" ;-) Famous last words again.
-
Rule 1 for unit tests: The changed lines may never mocked, and their in/out must be checked.
(if that is not followed, it is not a test, it is faking a test - a fraud, not a test)
Examples for of fake / fraudulent "make-believe" tests for such a sequence of mocks, including fdopen():
with patch("os.path.exists", return_value=file_exists), patch(
"os.makedirs"
) as mock_makedirs, patch("os.open", return_value=10), patch(
"os.fdopen", return_value=MagicMock(spec=open)
) as mock_fdopen:
api.lazy_complete_init()The above mocks fdopen() with the fake spec for open, which it is not - Or, even worse, but easier to get:
mocker.patch.object(os, "open")
mocker.patch.object(os, "fdopen")
api.lazy_complete_init()I've a mock-less, correct unit test that covers the changed method completely (and other parts), will send it tomorrow.
Edwin said:
So having a test that runs together with the commit hopefully avoids those kinds of mistakes.
I also remember reading from Edwin that an additional layer of defence before it hits in XenRT is better as well, from Rob that and unit tests are welcome.
I'll open a PR to include the accompanying unit test into this PR tomorrow, and update the PR.
This avoid the possibility of the
opentruncating an existing shared memory block, whichopen(<path>, 'wb')would do. Should have only happened on domain id wrap so is probably extremely rare/unlikely.