From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id 32A56E0082E; Mon, 6 Feb 2017 15:09:13 -0800 (PST) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on yocto-www.yoctoproject.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 X-Spam-HAM-Report: * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, * medium trust * [192.55.52.120 listed in list.dnswl.org] * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id 9C1B9E007AD; Mon, 6 Feb 2017 15:09:11 -0800 (PST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP; 06 Feb 2017 15:09:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,342,1477983600"; d="scan'208";a="930859736" Received: from unknown (HELO speedy) ([10.7.197.100]) by orsmga003.jf.intel.com with ESMTP; 06 Feb 2017 15:09:10 -0800 Message-ID: <1486422550.42955.166.camel@linux.intel.com> From: Todor Minchev To: Jianxun Zhang Date: Mon, 06 Feb 2017 15:09:10 -0800 In-Reply-To: References: <20170202223712.8164-1-todor.minchev@linux.intel.com> <20170202223712.8164-4-todor.minchev@linux.intel.com> X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Cc: meta-intel@yoctoproject.org, yocto@yoctoproject.org Subject: Re: [PATCH 3/3] rmc: add database extraction functionality X-BeenThere: yocto@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Discussion of all things Yocto Project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Feb 2017 23:09:13 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Mon, 2017-02-06 at 13:09 -0800, Jianxun Zhang wrote: > Todor, > Nice change overall. I haven’t run any test and just share multiple (11) inline comments for this patch. A patchset incorporating these comments is in progress. > 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 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 > > --- > > 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” Will do. > > > /* 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 > > #include > > #include > > @@ -14,8 +15,11 @@ > > #include > > #include > > > > -#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... 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. > > + */ OK > > +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. Makes sense > > + } > > + } > > + > > + /* 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. We can have the blobs associated with each fingerprint dumped in a directory with a predefined name and then store the fingerprint signature in a file in that directory. The user can then figure out what blobs correspond to what fingerprint. I am also going to get the signature dumped with the fingerprint dumper e.g Signature: xxxxxxxxxxxxxxxxxxxxxxx Finger 0 type : 0x01 Finger 0 offset : 0x05 Finger 0 name: : product_name Finger 0 value : Super Server *snip* > > + 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 > > #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. OK.. We'll do the database validation with is_rmcdb function. > > > > /* 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. OK > > 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. Alright, it will be 'rmc -E -d rmc.db -o out_dir'.. > > + "\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 \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... I was trying to stick to the 80 columns coding style, but since most of the lines are already more than 80 columns this probably makes no sense. > > 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... Same as above. > > 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 > > >