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

image_info: add -x option to extract segments to disk (ESPTOOL-947) #1023

Closed
wants to merge 1 commit into from

Conversation

sven337
Copy link

@sven337 sven337 commented Oct 24, 2024

This adds ability to extract the segments from an image to disk. This can be useful for later analysis or modification, and was easy enough to add.

Pass -x to image_info. Destination directory is /tmp, file naming contains some metadata to make life easier.

I have tested this change with the following hardware & software combinations:

ESP32 firmware

I have run the esptool.py automated integration tests with this change and the above hardware:

None yet

Copy link

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "image_info: add -x option to extract segments to disk":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello sven337, we appreciate your contribution to this project!


📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 2d2c374

@github-actions github-actions bot changed the title image_info: add -x option to extract segments to disk image_info: add -x option to extract segments to disk (ESPTOOL-947) Oct 24, 2024
@radimkarnis
Copy link
Collaborator

Hi @sven337,
thank you for taking the time to contribute!

Although this feature is useful for your specific use case, it is not demanded enough to be included in esptool.
To explain - we try to make esptool as lean as possible, not bloating it with options and arguments.

Since esptool is open source, it is very easy to hack into it and bend it to do other things. If esptool contained all these hacks, maintaining and using it would be a nightmare.

We appreciate every PR, but I have to close this.

@sven337
Copy link
Author

sven337 commented Oct 25, 2024

This is disappointing, as the feature is pretty small, and I don't see what justifies calling it a hack or introducing bloat given the size of the patch.

Can you please describe how you evaluate "demand" for the feature? Seems like a chicken and egg problem to me: the feature isn't there at present, so people have to write their custom scripts to implement it when they need it, and you're not told about it. I can file feature requests but I assume you would close them as not in scope for esptool (and that seems strictly inferior to contributing patches). Who defines the scope and how? Perhaps I can make a case.

The problem with having my own fork is that it reduces discoverability : unless I make a big effort to market my use case of firmware binary hacking, and the fork that supports it, other people with the same interest will have trouble identifying the prior art and will end up re-doing things from scratch.

I am sympathetic to questions of maintainability, but I thought esptool was meant to be a swiss-army knife for ESP firmwares, and it seems like a disservice to the ESP hobbyist community to claim that modifying an existing binary firmware isn't a legitimate endeavor worth exposing features for. For that particular purpose, esptool has some missing things (make_image for ESP32, see my other PR ; this very PR ; updating checksums in an existing firmware).

@dobairoland
Copy link
Collaborator

image_info prints information. One can capture it and redirect to files with or without modification. But if the segments need to be written out than it doesn't have anything to do with the command about showing general info about them. Therefore, it is out scope for image_info.

You don't have to maintain a fork of esptool. You can create your scripts which can import esptool and do all kinds of special operations you need. Or you can just use pyelftools without esptool. Or use some ELF file reader applications.

You hardcoded /tmp. This won't work on Windows. Bloating comes into the picture when you realize that we would need another argument to specify the file output name, probably another with some kind of format definition, and document the format. Then add another argument for filtering which segments to put into files. One has to do all this if this should be an universal feature for general use and not just your use case.

@sven337
Copy link
Author

sven337 commented Oct 25, 2024

I considered to implement "image_extract" instead of doing this as part of "image_info", but that would have been more code, so I thought it would be neater in image_info. If you disagree, I can definitely move this to "image_extract", and take care of "all the rest", if that will let you consider the PR.
I don't perceive that the segment file name format would need configurability or documentation, as it's self-explanatory, but here also I'll do what you ask if that helps get it considered.

On the other hand, if an extra commandline argument is considered bloat no matter what, and you're going to say no no matter what I improve, then I have nothing to say other than take note of your hostile stance towards use cases that probably shouldn't deserve your scorn.

To elaborate on the use case:
If you want to extend an existing segment to inject code, the ELF ecosystem is unsuitable for that.
The workflow I propose is uniquely simple: dump the segments, append to them, then re-create the image and everything works.
Doing this with an ELF intermediary requires using a third-party (or custom) script to generate an ELF from the image, then use an awfully involved custom script (I don't know of any third party preexisting script for that) to extend the section and program headers (a naive reading of objcopy's documentation will make you think it can do that, but it actually cannot, so you need custom code, llvm's objcopy has some better support for it but it's far from straightforward), then use elf2image.
This is gratuitously complex given the fundamental simplicity of the task at hand : in that case, ELF is just not purpose-appropriate, and enforcing the use of ELF is a disservice to the task at hand.

@dobairoland
Copy link
Collaborator

I can definitely move this to "image_extract", and take care of "all the rest", if that will let you consider the PR.

Please don't.

I'm sorry if we made you think we want esptool to go in this direction.

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.

3 participants