Skip to content
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

Unreachable fsync call in SysCommandWorker.write #2773

Open
correctmost opened this issue Nov 5, 2024 · 5 comments
Open

Unreachable fsync call in SysCommandWorker.write #2773

correctmost opened this issue Nov 5, 2024 · 5 comments

Comments

@correctmost
Copy link
Contributor

Description

pylint --enable=unreachable . picks up the following unreachable fsync call:

if self.child_fd:
return os.write(self.child_fd, data + (b'\n' if line_ending else b''))
os.fsync(self.child_fd)

Warning

************* Module archinstall.lib.general
archinstall/lib/general.py:219:3: W0101: Unreachable code (unreachable)
@Torxed
Copy link
Member

Torxed commented Nov 5, 2024

That's because pty.fork() secondary executor will be the thing that performs that call.
Maybe pylint can't handle forked code?

@correctmost
Copy link
Contributor Author

Hmm, mypy also flags that line as unreachable:

archinstall/lib/general.py:219: error: Statement is unreachable  [unreachable]

I tried this sample code to try to understand the semantics better:

import os
import pty

def write():
    return os.write(fd, b"foo")
    raise AssertionError
    os.fsync(fd)

pid, fd = pty.fork()
write()

The code doesn't seem to trigger the AssertionError, which makes me think the fsync call might not actually be performed.

I was thinking the archinstall code needed to be updated like the following, but I might be misunderstanding the existing command worker code:

 if self.child_fd: 
 	bytes_written = os.write(self.child_fd, data + (b'\n' if line_ending else b'')) 
 	os.fsync(self.child_fd)
        return bytes_written

@Torxed
Copy link
Member

Torxed commented Nov 6, 2024

That example has no check if the code is executed in the parent or forked child.

Both threads will call write(), whereas I check if we have a pid, and only the child process will have something there, the parent won't and thus skips that part of the code.

import pty
import random

pid, fd = pty.fork()

with open(f"/tmp/pid_{random.randint(0, 2000)}", "w") as fh:
    fh.write(f"pid: {pid}, fd: {fd}")

Both threads will execute, but with different values :)

@correctmost
Copy link
Contributor Author

Thanks, the forking part makes sense :).

I think the main item of confusion for me is how the os.fsync call on 219 can be reached after a return statement on line 218:

if self.child_fd:
return os.write(self.child_fd, data + (b'\n' if line_ending else b''))
os.fsync(self.child_fd)

My understanding is that the return would stop further execution regardless of whether the code is executed by the child or parent process.

We can suppress the pylint and mypy warnings for line 219, but I would want to make sure it's a true false positive. Thanks!

@Torxed
Copy link
Member

Torxed commented Nov 6, 2024

Ah yeah that return never happens, sorry about that one hehe.
We can either move that line up or remove it all together

@Torxed Torxed closed this as completed Nov 6, 2024
@Torxed Torxed reopened this Nov 6, 2024
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

No branches or pull requests

2 participants