Re: [PATCH v7 01/12] tools: mkeficapsule: rework the code a little bit
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
> > Reviewed-by: Simon Glass
> > ---
> > 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:
>
> : 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, _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
Re: [PATCH v7 01/12] tools: mkeficapsule: rework the code a little bit
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
Reviewed-by: Simon Glass
---
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:
: No such file or directory
Or use fprintf(stderr, ...).
+ return -1;
+ }
+ if (stat(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.
+ 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.
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, _stat) < 0) {
- printf("cannot determine the size of %s\n", bin);
- goto err_1;
- }
- data =
[PATCH v7 01/12] tools: mkeficapsule: rework the code a little bit
Abstract common routines to make the code easily understandable.
No functional change.
Signed-off-by: AKASHI Takahiro
Reviewed-by: Simon Glass
---
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);
+ return -1;
+ }
+ if (stat(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);
+ 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) {
+ 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, _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, , _size))
+ goto err;
+
+ /*
+* write a capsule file
+*/
f = fopen(path, "w");
if (!f) {
printf("cannot open %s\n", path);
- goto err_2;
+ goto err;
}
+
+ /*
+*
