[PATCH v7 01/12] tools: mkeficapsule: rework the code a little bit
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Nov 17 02:20:26 CET 2021
On Tue, Nov 16, 2021 at 01:19:10PM +0100, Heinrich Schuchardt wrote:
> On 11/16/21 05:32, AKASHI Takahiro wrote:
> > Abstract common routines to make the code easily understandable.
> > No functional change.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> > ---
> > tools/mkeficapsule.c | 223 ++++++++++++++++++++++++++++++-------------
> > 1 file changed, 159 insertions(+), 64 deletions(-)
> >
> > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> > index 4995ba4e0c2a..afdcaf7e7933 100644
> > --- a/tools/mkeficapsule.c
> > +++ b/tools/mkeficapsule.c
> > @@ -61,17 +61,122 @@ static void print_usage(void)
> > tool_name);
> > }
> >
> > +/**
> > + * read_bin_file - read a firmware binary file
> > + * @bin: Path to a firmware binary file
> > + * @data: Pointer to pointer of allocated buffer
> > + * @bin_size: Size of allocated buffer
> > + *
> > + * Read out a content of binary, @bin, into @data.
> > + * A caller should free @data.
> > + *
> > + * Return:
> > + * * 0 - on success
> > + * * -1 - on failure
> > + */
> > +static int read_bin_file(char *bin, void **data, off_t *bin_size)
> > +{
> > + FILE *g;
> > + struct stat bin_stat;
> > + void *buf;
> > + size_t size;
> > + int ret = 0;
> > +
> > + g = fopen(bin, "r");
> > + if (!g) {
> > + printf("cannot open %s\n", bin);
>
> Error messages should be written to stderr. Use perror(bin) to get a
> message like:
>
> <filename>: No such file or directory
>
> Or use fprintf(stderr, ...).
OK.
But remember that I have used printf() throughout mkeficapsule.c
even before this patch.
So I will add similar changes in patch#1 for consistency.
> > + return -1;
> > + }
> > + if (stat(bin, &bin_stat) < 0) {
> > + printf("cannot determine the size of %s\n", bin);
> > + ret = -1;
> > + goto err;
> > + }
> > + if (bin_stat.st_size > (u32)~0U) {
> > + printf("file size is too large: %s\n", bin);
> > + ret = -1;
> > + goto err;
> > + }
> > + buf = malloc(bin_stat.st_size);
>
> For a symbolic link st_size is the length of the pathname it contains.
> So this function is of no help here.
Not true.
According to stat() manpage,
"lstat() is identical to stat(), except that if pathname is a symbolic
link, then it returns information about the link itself, not the file
that it refers to."
So stat() is a right system call.
> > + if (!buf) {
> > + printf("cannot allocate memory: %zx\n",
> > + (size_t)bin_stat.st_size);
> > + ret = -1;
> > + goto err;
> > + }
> > +
> > + size = fread(buf, 1, bin_stat.st_size, g);
> > + if (size < bin_stat.st_size) {
>
> fread() reads from a stream which is not necessarily a file but can be a
> named pipe. stat() does not indicate when the stream will end.
>
> You have to run fread() in a loop until feof() or ferror() indicate that
> there is nothing more to be read from the stream.
No.
"a stream which is not necessarily a file but can be a named pipe"
may be true, but we don't handle such a use case with this tool.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > + printf("read failed (%zx)\n", size);
> > + ret = -1;
> > + goto err;
> > + }
> > +
> > + *data = buf;
> > + *bin_size = bin_stat.st_size;
> > +err:
> > + fclose(g);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * write_capsule_file - write a capsule file
> > + * @bin: FILE stream
> > + * @data: Pointer to data
> > + * @bin_size: Size of data
> > + *
> > + * Write out data, @data, with the size @bin_size.
> > + *
> > + * Return:
> > + * * 0 - on success
> > + * * -1 - on failure
> > + */
> > +static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg)
> > +{
> > + size_t size_written;
> > +
> > + size_written = fwrite(data, 1, size, f);
> > + if (size_written < size) {
> > + printf("%s: write failed (%zx != %zx)\n", msg,
> > + size_written, size);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * create_fwbin - create an uefi capsule file
> > + * @path: Path to a created capsule file
> > + * @bin: Path to a firmware binary to encapsulate
> > + * @guid: GUID of related FMP driver
> > + * @index: Index number in capsule
> > + * @instance: Instance number in capsule
> > + * @mcount: Monotonic count in authentication information
> > + * @private_file: Path to a private key file
> > + * @cert_file: Path to a certificate file
> > + *
> > + * This function actually does the job of creating an uefi capsule file.
> > + * All the arguments must be supplied.
> > + * If either @private_file ror @cert_file is NULL, the capsule file
> > + * won't be signed.
> > + *
> > + * Return:
> > + * * 0 - on success
> > + * * -1 - on failure
> > + */
> > static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> > unsigned long index, unsigned long instance)
> > {
> > struct efi_capsule_header header;
> > struct efi_firmware_management_capsule_header capsule;
> > struct efi_firmware_management_capsule_image_header image;
> > - FILE *f, *g;
> > - struct stat bin_stat;
> > - u8 *data;
> > - size_t size;
> > + FILE *f;
> > + void *data;
> > + off_t bin_size;
> > u64 offset;
> > + int ret;
> >
> > #ifdef DEBUG
> > printf("For output: %s\n", path);
> > @@ -79,25 +184,28 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> > printf("\tindex: %ld\n\tinstance: %ld\n", index, instance);
> > #endif
> >
> > - g = fopen(bin, "r");
> > - if (!g) {
> > - printf("cannot open %s\n", bin);
> > - return -1;
> > - }
> > - if (stat(bin, &bin_stat) < 0) {
> > - printf("cannot determine the size of %s\n", bin);
> > - goto err_1;
> > - }
> > - data = malloc(bin_stat.st_size);
> > - if (!data) {
> > - printf("cannot allocate memory: %zx\n", (size_t)bin_stat.st_size);
> > - goto err_1;
> > - }
> > + f = NULL;
> > + data = NULL;
> > + ret = -1;
> > +
> > + /*
> > + * read a firmware binary
> > + */
> > + if (read_bin_file(bin, &data, &bin_size))
> > + goto err;
> > +
> > + /*
> > + * write a capsule file
> > + */
> > f = fopen(path, "w");
> > if (!f) {
> > printf("cannot open %s\n", path);
> > - goto err_2;
> > + goto err;
> > }
> > +
> > + /*
> > + * capsule file header
> > + */
> > header.capsule_guid = efi_guid_fm_capsule;
> > header.header_size = sizeof(header);
> > /* TODO: The current implementation ignores flags */
> > @@ -105,70 +213,57 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> > header.capsule_image_size = sizeof(header)
> > + sizeof(capsule) + sizeof(u64)
> > + sizeof(image)
> > - + bin_stat.st_size;
> > -
> > - size = fwrite(&header, 1, sizeof(header), f);
> > - if (size < sizeof(header)) {
> > - printf("write failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > + + bin_size;
> > + if (write_capsule_file(f, &header, sizeof(header),
> > + "Capsule header"))
> > + goto err;
> >
> > + /*
> > + * firmware capsule header
> > + * This capsule has only one firmware capsule image.
> > + */
> > capsule.version = 0x00000001;
> > capsule.embedded_driver_count = 0;
> > capsule.payload_item_count = 1;
> > - size = fwrite(&capsule, 1, sizeof(capsule), f);
> > - if (size < (sizeof(capsule))) {
> > - printf("write failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > + if (write_capsule_file(f, &capsule, sizeof(capsule),
> > + "Firmware capsule header"))
> > + goto err;
> > +
> > offset = sizeof(capsule) + sizeof(u64);
> > - size = fwrite(&offset, 1, sizeof(offset), f);
> > - if (size < sizeof(offset)) {
> > - printf("write failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > + if (write_capsule_file(f, &offset, sizeof(offset),
> > + "Offset to capsule image"))
> > + goto err;
> >
> > + /*
> > + * firmware capsule image header
> > + */
> > image.version = 0x00000003;
> > memcpy(&image.update_image_type_id, guid, sizeof(*guid));
> > image.update_image_index = index;
> > image.reserved[0] = 0;
> > image.reserved[1] = 0;
> > image.reserved[2] = 0;
> > - image.update_image_size = bin_stat.st_size;
> > + image.update_image_size = bin_size;
> > image.update_vendor_code_size = 0; /* none */
> > image.update_hardware_instance = instance;
> > image.image_capsule_support = 0;
> > + if (write_capsule_file(f, &image, sizeof(image),
> > + "Firmware capsule image header"))
> > + goto err;
> >
> > - size = fwrite(&image, 1, sizeof(image), f);
> > - if (size < sizeof(image)) {
> > - printf("write failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > - size = fread(data, 1, bin_stat.st_size, g);
> > - if (size < bin_stat.st_size) {
> > - printf("read failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > - size = fwrite(data, 1, bin_stat.st_size, f);
> > - if (size < bin_stat.st_size) {
> > - printf("write failed (%zx)\n", size);
> > - goto err_3;
> > - }
> > -
> > - fclose(f);
> > - fclose(g);
> > - free(data);
> > -
> > - return 0;
> > + /*
> > + * firmware binary
> > + */
> > + if (write_capsule_file(f, data, bin_size, "Firmware binary"))
> > + goto err;
> >
> > -err_3:
> > - fclose(f);
> > -err_2:
> > + ret = 0;
> > +err:
> > + if (f)
> > + fclose(f);
> > free(data);
> > -err_1:
> > - fclose(g);
> >
> > - return -1;
> > + return ret;
> > }
> >
> > /*
> >
>
More information about the U-Boot
mailing list