Implement sparse image support (no deps on libsparse) #37
Conversation
|
@bmx666 Hi, Why is this PR a draft? Are there any flaws in this implementation? |
|
Hi @parport0
I'm waiting feedback from maintainer...
There are only two flaws:
|
|
Looks pretty good to me, thanks @bmx666 |
Hi @obbardc , I uploaded an update to support |
|
@lumag I'd really love to see proper sparse support in qdl. Are you interested in that ? |
|
@bmx666 can you rebase this, please? |
|
Sorry for not commenting on this earlier. I think this PR looks good, but I wasn't able to validate the functionality - assuming PEBKAC I was going to get back to this, but didn't. Given @bmx666 announcement that he doesn't have access to hardware it would be nice if someone with understanding of the sparse format/mechanism could conclude this effort (with appropriate credit to @bmx666 of course). |
I didn't see that. I'll try to take a look (assuming at @bmx666 is ok with it) |
bmx666
force-pushed
the
sparse
branch
2 times, most recently
from
ebfa696 to
92fd16d
Compare
Hi @obbardc , done! I didn't notice any serious conflicts, build the current branch on my side.
@andersson Sparse format is pretty simple Sparse header contains version, checksum, size of non-sparse image and number of chunks.
If the XML does not contain the partition attribute |
| @@ -12,12 +12,16 @@ struct program { | |||
| const char *label; | |||
| unsigned num_sectors; | |||
| unsigned partition; | |||
| bool sparse; | |||
There was a problem hiding this comment.
| bool sparse; | |
| bool is_sparse; |
nitpick: wonder if it makes sense to put these with is_nand & is_erase ?
There was a problem hiding this comment.
Not sure, "sparse" naming reflects a real XML-attribute "sparse", same as "label", "num_sectors" and etc
| @@ -0,0 +1,48 @@ | |||
| #ifndef __SPARSE_H__ | |||
There was a problem hiding this comment.
Does this header need a copyright ?
There was a problem hiding this comment.
I don't know what it's supposed to be.
There was a problem hiding this comment.
The current file is based on original header from AOSP - https://android.googlesource.com/kernel/lk/+/5c761131621480d1288692a554805c8cc3fb5442%5E!/
Looks the same in the latest version - https://android.googlesource.com/platform/system/core/+/master/libsparse/sparse_format.h
Signed-off-by: Maxim Paymushkin <[email protected]>
Signed-off-by: Maxim Paymushkin <[email protected]>
@obbardc done! |
program.c
Outdated
| unsigned start_sector, chunk_size, chunk_type, chunk_data; | ||
|
|
||
| if (sparse_header_parse(fd, &sparse_header)) { | ||
| /* ignore false sparse images */ |
There was a problem hiding this comment.
Isn't this comment misleading? If I read the code correctly we'll be programming the referenced image as a normal (non-sparse) image, not ignoring it.
There was a problem hiding this comment.
updated to
/*
* If the XML tag "program" contains the attribute 'sparse="true"'
* for a partition node but lacks a sparse header,
* it will be validated against the defined partition size.
* If the sizes match, it is likely that the 'sparse="true"' attribute
* was set by mistake.
*/
program.c
Outdated
| } else { | ||
| program_sparse = program_load_sparse(program, fd); | ||
| if (!program_sparse) { | ||
| fprintf(stderr, "[PROGRAM] load sparse failed\n"); |
There was a problem hiding this comment.
I believe you printed an (more useful) error in each error path within program_load_sparse() already, so no need to tell the user once more.
There was a problem hiding this comment.
Hmm, you can generate a correct sparse image, but all chunks will be DON'T CARE or the chunk header will contain zero bytes of data.
On the one hand, the sparse image is validated, but on the other hand, there is nothing to flash and the program will have to be skipped.
Maybe you can suggest a better scenario?
Signed-off-by: Maxim Paymushkin <[email protected]>
Hi @andersson, yep, it is another "sparse" version but without deps on libsparse. I've been testing it on several QCOM projects for over 2 years now, no problem.