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

Add close() and context management to ubi_file #89

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

AT0myks
Copy link
Contributor

@AT0myks AT0myks commented Jun 1, 2023

Add a new method to ubi_file which currently doesn't have a way to close the underlying file handle. Use it in the scripts where instances are created.

Context management allows for a cleaner way to work with ubi_file instances.

@qkaiser
Copy link
Contributor

qkaiser commented Jun 1, 2023

Could you provide a concise description of what this PR tries to achieve both in the PR description and in the git commit message ?

Thanks :)

Add a new method to ubi_file which currently doesn't have a way to close
the underlying file handle. Use it in the scripts where a ubi_file
instance is created.

Context management allows for a cleaner way to work with ubi_file
instances.
@AT0myks
Copy link
Contributor Author

AT0myks commented Jun 1, 2023

Didn't bother to write one because I think the title is pretty self-explanatory but here you go. I'm sure if you take a look at the few added lines you'll see what this PR does.

Copy link
Contributor

@qkaiser qkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is okay. Minimal impact since the OS close the file handle anyway when the process exits (which happens immediately after the explicit close in modified scripts). Agreed that explicit closing is better though.

@vlaci ok to merge ?

@qkaiser
Copy link
Contributor

qkaiser commented Jun 2, 2023

BTW, I did some strace -e openat,close -f on ubireader scripts to check this branch and omg I can't wait for us to do some rewrite in #79. The ubireader_extract_images opens the input file four times.

@qkaiser qkaiser self-assigned this Jun 2, 2023
@qkaiser qkaiser requested a review from vlaci June 2, 2023 06:03
@vlaci vlaci merged commit cc7225e into onekey-sec:main Jun 8, 2023
1 check passed
AT0myks added a commit to AT0myks/reolink-fw that referenced this pull request Jun 29, 2023
While waiting for the next version of ubireader that will have
onekey-sec/ubi_reader#89, use a context manager to close the UBI file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants