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

imgtool: Extend functionality with getpubhash and output to file #1752

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

bence-balogh
Copy link
Contributor

  • Added getpubhash: It returns the sha256 hash of the public key. It can be useful if MCUBOOT_HW_KEY is set and only the hash has to be written to the device.
  • Added functionality to getpub and getpubhash to print to a file instead of stdout.
  • Added functionality to getpub and getpubhash to print the output as a raw value.

scripts/imgtool/main.py Outdated Show resolved Hide resolved
scripts/imgtool/main.py Outdated Show resolved Hide resolved
scripts/imgtool/keys/general.py Outdated Show resolved Hide resolved
print("\n" + trailer, file=file)
if len_format is not None:
print(len_format.format(len(encoded_bytes)), file=file)
sys.stdout.close = lambda: None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain it to me? Thanks! :)
Is it possible that it's not necessary if the the default file=sys.stdout is removed for the "emit functions" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sys.stdout does not have the close method implemented and would throw an error when the with (open...'s scope ends. This way we add an empty method to prevent this error and there's no need for additional checks whether the file is sys.stdout or a regular file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tried it without the sys.stdout.close = lambda: None line and it worked fine. Isn't it that with the is not sys.stdout check it's avoided?

Copy link
Contributor Author

@bence-balogh bence-balogh Jul 28, 2023

Choose a reason for hiding this comment

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

You are right, my reasoning was not correct. 😬
sys.stdout does have a close() method but once it's called, the print() will no longer work until the sys.stdout's open() is called again.
So a print() after the close() would cause a crash if this sys.stdout.close = lambda: None is removed so as a workaround its close() is overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this solution from the latest patchset because it'd change the global state and it's not that clear either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update.
Another solution could have been to use the open()/close() methods directly, without with.

scripts/imgtool/main.py Outdated Show resolved Hide resolved
scripts/imgtool/keys/general.py Outdated Show resolved Hide resolved
scripts/imgtool/keys/general.py Outdated Show resolved Hide resolved
scripts/imgtool/keys/general.py Show resolved Hide resolved
scripts/imgtool/keys/general.py Outdated Show resolved Hide resolved
docs/release-notes.d/imgtool-getpub-hash.md Outdated Show resolved Hide resolved
@bence-balogh bence-balogh force-pushed the imgtool-get-pub-addition branch 3 times, most recently from 00761c8 to ca29872 Compare July 27, 2023 07:23
@davidvincze
Copy link
Collaborator

Please rebase your PR, Zephyr build was broken, but it's been fixed by: #1755

@bence-balogh bence-balogh force-pushed the imgtool-get-pub-addition branch 3 times, most recently from da68716 to 650aaed Compare August 1, 2023 11:42
@davidvincze
Copy link
Collaborator

#nitpick
MCUboot tends the follow Python's PEP8 style guide. I didn't want to comment on some minor warnings because you just followed the existing "patterns", but now new types of errors have been introduced in the scripts/imgtool/keys/general.py file. May I ask you to solve these?

imgtool/keys/general.py:12:80: E501 line too long (94 > 79 characters)
imgtool/keys/general.py:14:33: E272 multiple spaces before keyword
imgtool/keys/general.py:15:80: E501 line too long (94 > 79 characters)
imgtool/keys/general.py:17:80: E501 line too long (96 > 79 characters)
imgtool/keys/general.py:19:80: E501 line too long (88 > 79 characters)
imgtool/keys/general.py:34:80: E501 line too long (88 > 79 characters)
imgtool/keys/general.py:38:80: E501 line too long (96 > 79 characters)
imgtool/keys/general.py:45:80: E501 line too long (93 > 79 characters)
imgtool/keys/general.py:49:80: E501 line too long (101 > 79 characters)
imgtool/keys/general.py:54:34: E272 multiple spaces before keyword
imgtool/keys/general.py:70:80: E501 line too long (88 > 79 characters)
imgtool/keys/general.py:78:33: E272 multiple spaces before keyword

You can check these using either the pycodestyle or flake8 tools: https://realpython.com/python-pep8/#linters

Signed-off-by: Dávid Házi <david.hazi@arm.com>
Signed-off-by: Bence Balogh <bence.balogh@arm.com>
Change-Id: I6028955be5cbcd20d49ef2126dce8d4636b824a6
Signed-off-by: Dávid Házi <david.hazi@arm.com>
Signed-off-by: Bence Balogh <bence.balogh@arm.com>
Change-Id: Ia7f385e5e1b0471aae7693baa54e9a385ad3ae3f
Signed-off-by: Dávid Házi <david.hazi@arm.com>
Signed-off-by: Bence Balogh <bence.balogh@arm.com>
Change-Id: I91d5c07c1bb2b8abe2592cd49b2053c881465ba2
Signed-off-by: Bence Balogh <bence.balogh@arm.com>
Change-Id: I48eabb1dc9696ef50d12fc8782616169ba8acc45
@davidvincze davidvincze merged commit 70acc41 into mcu-tools:main Aug 8, 2023
54 checks passed
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.

2 participants