Skip to content

Conversation

@MarkSymsCtx
Copy link
Contributor

This avoid the possibility of the open truncating an existing shared memory block, which open(<path>, 'wb') would do. Should have only happened on domain id wrap so is probably extremely rare/unlikely.

Copy link
Member

@psafont psafont left a 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.

@acefei
Copy link
Contributor

acefei commented Jan 31, 2024

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>
@codecov
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@115339b). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5405   +/-   ##
=========================================
  Coverage          ?   45.38%           
=========================================
  Files             ?       18           
  Lines             ?     2937           
  Branches          ?        0           
=========================================
  Hits              ?     1333           
  Misses            ?     1604           
  Partials          ?        0           
Flag Coverage Δ
python2.7 52.09% <0.00%> (?)
python3.11 50.15% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…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>
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a 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 file

The 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>
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants