[coreboot-gerrit] [M] Change in coreboot[master]: cbfstool: Add comment to define stability rules for cbfstool print -k

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/74347 )

Change subject: cbfstool: Add comment to define stability rules for cbfstool print -k
......................................................................

cbfstool: Add comment to define stability rules for cbfstool print -k

In CB:41119, I sort of made up a mechanism on the fly for how to make
the machine-parseable cbfstool print output extensible without breaking
backwards compatibility for older scripts. But I only explained it in
the commit message which is not very visible. This patch adds a comment
to the function that generates that output so that people who want to
change it can understand the intent.

Signed-off-by: Julius Werner 
Change-Id: I0d18d59e7fe407eb34710d6a583cfae667723eb7
Reviewed-on: https://review.coreboot.org/c/coreboot/+/74347
Tested-by: build bot (Jenkins) 
Reviewed-by: Yu-Ping Wu 
---
M util/cbfstool/cbfs_image.c
1 file changed, 52 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Yu-Ping Wu: Looks good to me, approved

diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c
index 683a96c..7a34bfe 100644
--- a/util/cbfstool/cbfs_image.c
+++ b/util/cbfstool/cbfs_image.c
@@ -730,6 +730,12 @@

const char *name = header->filename;

+	/* This is so special rows in cbfstool print -k -v output stay unambiguous. */
+	if (name[0] == '[') {
+		ERROR("CBFS file name `%s` must not start with `[`\n", name);
+		return -1;
+	}
+
    uint32_t entry_type;
    uint32_t addr, addr_next;
    struct cbfs_file *entry, *next;
@@ -1497,6 +1503,32 @@
    return 0;
 }

+/*
+ * The format of this output has been stable for many years. Since it is meant
+ * to be parsed by scripts, we should probably not lightly make changes to it as
+ * that could break older scripts expecting a different format.
+ *
+ * Until CB:41119, the `-v` flag made no difference when `-k` was selected, so
+ * presumably no scripts were using that combination. That's why that patch left
+ * the output for `-k` by itself alone to avoid breaking legacy scripts, and
+ * expanded `-k -v` to allow an arbitrary number of `<key>:<value>` tokens at
+ * the end of each row behind the legacy column output. So the new output format
+ * stability rules should be that `-k` will stay as it is, and `-k -v` may be
+ * expanded to add more `<key>:<value>` tokens to the end of a row. Scripts that
+ * want to parse `-k -v` output should be written to gracefully ignore any extra
+ * such tokens where they don't recognize the key.
+ *
+ * The `-k -v` output may also include extra rows that start with a `[`. These
+ * do not represent a CBFS file and can instead be used to display data that is
+ * associated with the CBFS as a whole and not any single file. Currently
+ * defined are `[FMAP REGION]\t<region name>` and
+ * `[METADATA HASH]\t<hash>:<algo>`. More may be defined in the future and
+ * scripts parsing `-k -v` output should be written to gracefully ignore any
+ * rows starting with `[` that they don't recognize.
+ *
+ * The format for existing `key:value` tokens or `[` rows should never be
+ * changed once they are added.
+ */
 static int cbfs_print_parseable_entry_info(struct cbfs_image *image,
    				struct cbfs_file *entry, void *arg)
 {