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 <todor.minc...@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.minc...@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”
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 <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...
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 <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.
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 <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...
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
> >
>
--
_______________________________________________
yocto mailing list
yocto@yoctoproject.org
https://lists.yoctoproject.org/listinfo/yocto
