qdl: decouple helper modules from the CLI front-end - #313
Open
igoropaniuk wants to merge 7 commits into
Open
Conversation
qdl.c has grown into the tool's junk drawer, and the Sahara programmer plumbing is its largest self-contained tenant: the specifier-to-image mapping, the CPIO archive decoder and, since the sahara-archive subcommand arrived, the matching writer. None of it depends on the CLI front-end it lives in, but burying it there couples it to main()'s compilation unit, keeps the decoder and writer of the same format at opposite ends of a 1500-line file, and puts the logic out of unit tests' reach. Move the lot into programmer.c, where the format has one home and the front-end shrinks to argument handling. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
The archive decoder parses an untrusted blob with hand-rolled bounds checks, and the audit found real holes there - unvalidated image ids and entry names among them. Those hardened paths could previously only be exercised by handing qdl a corrupt archive on the command line. With the decoder extracted into its own module, craft newc archives in memory and pin the contract: a valid archive maps its images, and a truncated header, an out-of-range or zero image id and an unterminated name are each rejected without touching the image table. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
decode_backend() (a --backend value to enum) and qdl_split_specifier() (splitting "<file>::<selector>") were static string parsers trapped in qdl.c, which cannot be unit-tested because qdl.c defines main(). Move them to util.c alongside the other argument parsers (parse_storage_ address, decode_storage_type) and declare them in qdl.h. Pure code move, no behaviour change. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
Cover the pure helpers in util.c that had no test: the storage address parser (partition, partition/sector, sector+length, and the bare-name / partition-name GPT forms, plus the rejected forms), the storage-type encode/decode round-trip, and the XML attribute accessors - including that a present-but-empty attribute yields NULL without raising an error, which several load paths rely on. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
Extend the util test suite with the two parsers just moved out of qdl.c: backend-name decoding (auto/usb/qud, NULL defaulting to auto, and an unknown name) and specifier splitting (plain filename, file::selector, and the malformed empty / double-"::" / empty-part forms). Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
How a positional argument is classified - verb, rawprogram, patch, read, ufs or contents document - decides the whole shape of a flashing run, yet the logic lived as private helpers of the CLI front-end where no test could reach it, and misclassification bugs (an erase-only rawprogram failing as "unknown file type") could only be found by flashing. Give the classification its own module so it can be tested directly. The contents.xml sniff used by the sahara-archive subcommand is the same kind of decision and moves along with it. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
Cover detect_type(): the command verbs (read/write/erase/flash/ sha256), an argument that is neither a verb nor an existing file, the recognized XML roots (patches, data with program/read/ufs children, contents), a well-formed document with an unrecognized root or child resolving to QDL_FILE_UNKNOWN, and malformed XML being rejected. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
qdl.cmixes the CLI front-end with logic that has nothing to do withargument handling, and because it defines main(), everything trapped
in it is out of unit tests' reach. Carve out three self-contained
modules, each landing with its own test suite:
programmer.c: the Sahara programmer plumbing - the specifier-to-imagemapping and the CPIO archive decoder and writer, which lived hundreds
of lines apart despite implementing the same format. The audit-hardened
decoder paths (image id validation, truncation, malformed names) are
now pinned by unit tests crafting newc archives in memory.
util.cgainsdecode_backend()andqdl_split_specifier(), joining theother argument parsers, with tests for both plus the existing helpers.
input_type.c: classification of positional inputs - the command verbsand the XML file types - together with the
contents.xmlsniff used bythe sahara-archive subcommand. Tests cover every verb (reset included)
and file type.
Pure code movement plus tests; no behavior change intended. qdl.c
shrinks from 1584 to 1109 lines.