[yocto] [PATCH 3/3] rmc: add database extraction functionality
Jianxun Zhang
jianxun.zhang at linux.intel.com
Mon Feb 6 13:09:52 PST 2017
Todor,
Nice change overall. I haven’t run any test and just share multiple (11) inline comments for this patch.
This should be the last one in the series. Please let me know if I missed any other RMC patches for review.
I plan to run rmc internal test once we have an updated patch set. We could need to add a new test case for the dumping feature in the future.
You can refer to the README in rmc project for the pipeline of merging.
Thanks!
> On Feb 2, 2017, at 2:37 PM, Todor Minchev <todor.minchev at linux.intel.com> wrote:
>
> The contents of an existing database file can be extracted in the
> current working directory with the -E option. The top level of the
> directory tree is rmc_db_dump and all files corresponding to
> a given record will be saved in a separate sub-directory. The sub-directory
> name of each record is the signature corresponding to the fingerprint for
> that record.
>
> Example:
> ./src/rmc -E -d rmc.db
>
> Successfully extracted rmc.db
>
> Signed-off-by: Todor Minchev <todor.minchev at linux.intel.com>
> ---
> inc/rmc_api.h | 9 ++++++
> src/lib/api.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++--
> src/lib/common/rmcl.c | 3 +-
> src/rmc.c | 44 +++++++++++++++++++-------
> 4 files changed, 126 insertions(+), 15 deletions(-)
>
> diff --git a/inc/rmc_api.h b/inc/rmc_api.h
> index a484389..ce26220 100644
> --- a/inc/rmc_api.h
> +++ b/inc/rmc_api.h
> @@ -74,6 +74,15 @@ extern int rmc_query_file_by_fp(rmc_fingerprint_t *fp, char *db_pathname, char *
> */
> extern int rmc_gimme_file(char* db_pathname, char *file_name, rmc_file_t *file);
>
> +
> +/* extract the contents of a database file and store the files corresponding to
> + * each record in a separate directory. The name of each directory is the signature
> + * of the fingerpring for that record
> + * (in) db_pathname: The path and file name of a RMC database file generated by RMC tool
> + * return: 0 on success, non-zero on failure.
> + */
> +int dump_db(char *db_pathname) ;
> +
Please move this into section 1.3, somewhere after next line. It doesn’t belong to section 1.2 “double-action API”
> /* 1.3 - Helper APIs */
>
> /* Free allocated data referred in a fingerprint
> diff --git a/src/lib/api.c b/src/lib/api.c
> index 0adb390..aca8d99 100644
> --- a/src/lib/api.c
> +++ b/src/lib/api.c
> @@ -3,6 +3,7 @@
> * RMC API implementation for Linux user space
> */
>
> +#define _GNU_SOURCE
> #include <stdio.h>
> #include <unistd.h>
> #include <errno.h>
> @@ -14,8 +15,11 @@
> #include <rmcl.h>
> #include <rsmp.h>
>
> -#define EFI_SYSTAB_PATH "/sys/firmware/efi/systab"
> -#define SYSTAB_LEN 4096 /* assume 4kb is enough...*/
> +#define EFI_SYSTAB_PATH "/sys/firmware/efi/systab"
> +#define SYSTAB_LEN 4096 /* assume 4kb is enough...*/
> +#define DB_DUMP_DIR "./rmc_db_dump" /* directory to store db data dump */
> +
> +extern const rmc_uint8_t rmc_db_signature[RMC_DB_SIG_LEN];
We could have a new small helper API like is_rmcdb(db_blob) in RMCL to hold checker logic at line 357 in this file, so that we can get rid of this line and make the checker reusable. (So far I feel the checker should work in both EFI and Linux contexts.)
We could even update checker API without bothering its callers in the future. Let me know if it makes sense...
>
> int read_file(const char *pathname, char **data, rmc_size_t* len) {
> int fd = -1;
> @@ -325,3 +329,80 @@ int rmc_gimme_file(char* db_pathname, char *file_name, rmc_file_t *file) {
>
> return ret;
> }
> +
> +/*
> + * Dump contents of database file
> + * (in) rmc_db - input database file to extract
rmc_db VS db_pathname. I think we can remove the comment here, it is already in the header file.
> + */
> +int dump_db(char *db_pathname) {
> + rmc_meta_header_t meta_header;
> + rmc_db_header_t *db_header = NULL;
> + rmc_record_header_t record_header;
> + rmc_uint64_t record_idx = 0; /* offset of each reacord in db*/
> + rmc_uint64_t meta_idx = 0; /* offset of each meta in a record */
> + rmc_uint64_t file_idx = 0; /* offset of file in a meta */
> + rmc_file_t file;
> + char *out_dir = NULL, *out_name = NULL;
> + rmc_size_t db_len = 0;
> + rmc_uint8_t *rmc_db = NULL;
> + struct stat s = {0};
> +
> + if (read_file(db_pathname, (char **)&rmc_db, &db_len)) {
> + fprintf(stderr, "Failed to read database file\n\n");
> + return 1;
> + }
> +
> + db_header = (rmc_db_header_t *)rmc_db;
> +
> + /* sanity check of db */
> + if (strncmp((const char *)db_header->signature,
> + (const char *)rmc_db_signature, RMC_DB_SIG_LEN))
> + return 1;
> +
> + /* create the top level directory */
> + if (stat(DB_DUMP_DIR, &s) == -1) {
> + if(mkdir(DB_DUMP_DIR, 0755)) {
> + fprintf(stderr, "Failed to create %s directory\n\n", out_name);
I think we should not go any further when we cannot create the top dir.
> + }
> + }
> +
> + /* query the meta. idx: start of record */
> + record_idx = sizeof(rmc_db_header_t);
> + while (record_idx < db_header->length) {
> + memcpy(&record_header, rmc_db + record_idx,
> + sizeof(rmc_record_header_t));
> +
> + /* directory name is fingerprint signature */
> + asprintf(&out_dir, "%s/%s/", DB_DUMP_DIR, record_header.signature.raw);
Technically, what we get from firmware could contain any kinds of chars. I guess there are some corner cases when chars are not accepted in a dir name.
> + if (stat(out_dir, &s) == -1) {
> + if(mkdir(out_dir, 0755)) {
> + fprintf(stderr, "Failed to create %s directory\n\n", out_name);
out_name -> out_dir
> + }
> + }
> +
> + /* find meta */
> + for (meta_idx = record_idx + sizeof(rmc_record_header_t);
> + meta_idx < record_idx + record_header.length;) {
> + memcpy(&meta_header, rmc_db + meta_idx, sizeof(rmc_meta_header_t));
> + file_idx = meta_idx + sizeof(rmc_meta_header_t);
> + rmc_ssize_t name_len = strlen((char *)&rmc_db[file_idx]) + 1;
> + file.blob = &rmc_db[file_idx + name_len];
> + file.blob_len = meta_header.length - sizeof(rmc_meta_header_t) -
> + name_len;
> + file.next = NULL;
> + file.type = RMC_GENERIC_FILE;
> + asprintf(&out_name, "%s%s", out_dir, (char *)&rmc_db[file_idx]);
> + /* write file to dump directory */
> + if (write_file((const char *)out_name, file.blob, file.blob_len, 0))
> + return 1;
> +
> + /* next meta */
> + meta_idx += meta_header.length;
> + free(out_name);
> + }
> + /* next record */
> + record_idx += record_header.length;
> + free(out_dir);
> + }
> + return 0;
> +}
> diff --git a/src/lib/common/rmcl.c b/src/lib/common/rmcl.c
> index 67622a0..c996577 100644
> --- a/src/lib/common/rmcl.c
> +++ b/src/lib/common/rmcl.c
> @@ -10,7 +10,7 @@
> #include <rmc_util.h>
> #endif
>
> -static const rmc_uint8_t rmc_db_signature[RMC_DB_SIG_LEN] = {'R', 'M', 'C', 'D', 'B'};
> +const rmc_uint8_t rmc_db_signature[RMC_DB_SIG_LEN] = {'R', 'M', 'C', 'D', 'B’};
Refer to my first comment. And signature should be internal in rmcl.
>
> /* compute a finger to signature which is stored in record
> * (in) fingerprint : of board, usually generated by rmc tool and rsmp
> @@ -242,7 +242,6 @@ int query_policy_from_db(rmc_fingerprint_t *fingerprint, rmc_uint8_t *rmc_db, rm
> policy->blob_len = meta_header.length - sizeof(rmc_meta_header_t) - cmd_name_len;
> policy->next = NULL;
> policy->type = type;
> -
I usually insert a new line before a return in rmc project as a loose rule, assuming this is an intentional change.
> return 0;
> }
> }
> diff --git a/src/rmc.c b/src/rmc.c
> index a051ccf..888cbdb 100644
> --- a/src/rmc.c
> +++ b/src/rmc.c
> @@ -32,6 +32,8 @@
> "running on\n" \
> "\t-d: database file to be queried\n" \
> "\t-o: path and name of output file of a specific command\n\n" \
> + "-E: Extract database data to current working directory\n” \
I believe user should be able to specify the output dir. DB_DUMP_DIR can serve as the default.
> + "\t-d: database file to extract\n\n" \
> "Examples (Steps in an order to add board support into rmc):\n\n" \
> "1. Generate board fingerprint:\n" \
> "\t./rmc -F\n\n" \
> @@ -49,11 +51,12 @@
> #define RMC_OPT_CAP_R (1 << 1)
> #define RMC_OPT_CAP_D (1 << 2)
> #define RMC_OPT_CAP_B (1 << 3)
> -#define RMC_OPT_F (1 << 4)
> -#define RMC_OPT_O (1 << 5)
> -#define RMC_OPT_B (1 << 6)
> -#define RMC_OPT_D (1 << 7)
> -#define RMC_OPT_I (1 << 8)
> +#define RMC_OPT_CAP_E (1 << 4)
> +#define RMC_OPT_F (1 << 5)
> +#define RMC_OPT_O (1 << 6)
> +#define RMC_OPT_B (1 << 7)
> +#define RMC_OPT_D (1 << 8)
> +#define RMC_OPT_I (1 << 9)
>
> static void usage () {
> fprintf(stdout, USAGE);
> @@ -312,7 +315,7 @@ int main(int argc, char **argv){
> /* parse options */
> opterr = 0;
>
> - while ((c = getopt(argc, argv, "FRD:B:b:f:o:i:d:")) != -1)
> + while ((c = getopt(argc, argv, "FRED:B:b:f:o:i:d:")) != -1)
> switch (c) {
> case 'F':
> options |= RMC_OPT_CAP_F;
> @@ -320,6 +323,9 @@ int main(int argc, char **argv){
> case 'R':
> options |= RMC_OPT_CAP_R;
> break;
> + case 'E':
> + options |= RMC_OPT_CAP_E;
> + break;
> case 'D':
> /* we don't know number of arguments for this option at this point,
> * allocate array with argc which is bigger than needed. But we also
> @@ -393,8 +399,8 @@ int main(int argc, char **argv){
> break;
> case '?':
> if (optopt == 'F' || optopt == 'R' || optopt == 'D' || optopt == 'B' || \
> - optopt == 'b' || optopt == 'f' || optopt == 'o' || optopt == 'd' \
> - || optopt == 'i')
> + optopt == 'E' || optopt == 'b' || optopt == 'f' || \
> + optopt == 'o' || optopt == 'd' || optopt == 'i')
> fprintf(stderr, "\nWRONG USAGE: -%c\n\n", optopt);
> else if (isprint(optopt))
> fprintf(stderr, "Unknown option `-%c'.\n\n", optopt);
> @@ -436,6 +442,13 @@ int main(int argc, char **argv){
> return 1;
> }
>
> + /* sanity check for -E */
> + if ((options & RMC_OPT_CAP_E) && (!(options & RMC_OPT_D))) {
> + fprintf(stderr, "\nERROR: -E requires -d <database file name>\n\n");
> + usage();
> + return 1;
> + }
> +
> /* sanity check for -B */
> if ((options & RMC_OPT_CAP_B) && (!(options & RMC_OPT_D) || !(options & RMC_OPT_O))) {
> fprintf(stderr, "\nWRONG: -B requires -d and -o\n\n");
> @@ -448,7 +461,8 @@ int main(int argc, char **argv){
> rmc_file_t file;
>
> if (!output_path) {
> - fprintf(stderr, "-B internal error, with -o but no output pathname specified\n\n");
> + fprintf(stderr, "-B internal error, with -o but no output \
> + pathname specified\n\n”);
Irrelevant change...
> goto main_free;
> }
>
> @@ -456,14 +470,22 @@ int main(int argc, char **argv){
> goto main_free;
>
> if (write_file(output_path, file.blob, file.blob_len, 0)) {
> - fprintf(stderr, "-B failed to write file %s to %s\n\n", input_blob_name, output_path);
> + fprintf(stderr, "-B failed to write file %s to %s\n\n",
> + input_blob_name, output_path);
Irrelevant change...
> rmc_free_file(&file);
> goto main_free;
> }
> -
> rmc_free_file(&file);
> }
>
> + /* dump database data */
> + if (options & RMC_OPT_CAP_E) {
> + if(dump_db(input_db_path_d))
> + fprintf(stderr, "\nFailed to extract %s\n\n", input_db_path_d);
> + else
> + printf("\nSuccessfully extracted %s\n\n", input_db_path_d);
> + }
> +
> /* generate RMC database file */
> if (options & RMC_OPT_CAP_D) {
> int record_idx = 0;
> --
> 2.11.0
>
More information about the yocto
mailing list