| Message ID | 20230616113426.13976-2-stefan.herbrechtsmeier-oss@weidmueller.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | Extend mkeficapsule tool to pack multiple payloads | expand |
On 6/16/23 13:34, Stefan Herbrechtsmeier wrote: > From: Malte Schmidt <malte.schmidt@weidmueller.com> Thanks for considering which parameters may be constants. nits: The Urban Dictionary defines 'constify' as: "To constantly do something, like constantly watching anime all day." %s/constify function parameters/make function parameters const/ > > Use const keyword for function parameters where appropriate. > > Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com> > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > --- > > tools/mkeficapsule.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > index 52be1f122e..b8db00b16b 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -88,8 +88,8 @@ static void print_usage(void) > * are filled in by create_auth_data(). > */ > struct auth_context { > - char *key_file; > - char *cert_file; > + const char *key_file; > + const char *cert_file; > uint8_t *image_data; > size_t image_size; > struct efi_firmware_image_authentication auth; > @@ -112,7 +112,7 @@ static int dump_sig; > * * 0 - on success > * * -1 - on failure > */ > -static int read_bin_file(char *bin, uint8_t **data, off_t *bin_size) > +static int read_bin_file(const char *bin, uint8_t **data, off_t *bin_size) > { > FILE *g; > struct stat bin_stat; > @@ -170,7 +170,8 @@ err: > * * 0 - on success > * * -1 - on failure > */ > -static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg) > +static int write_capsule_file(FILE *f, const void *data, size_t size, Why should size not be constant? The name of the parameter 'size' does not match the function documentation which has 'bin_size'. Please, change either of both. Parameter 'f' does not match the documentation which has 'bin'. For each function that you touch, please, ensure that the function parameters are correctly documented. > + const char *msg) > { > size_t size_written; > > @@ -343,7 +344,8 @@ static int create_auth_data(struct auth_context *ctx) > * * 0 - on success > * * -1 - on failure > */ > -static int dump_signature(const char *path, uint8_t *signature, size_t sig_size) > +static int dump_signature(const char *path, const uint8_t *signature, > + size_t sig_size) Why should sig_size not be constant? > { > char *sig_path; > FILE *f; > @@ -402,10 +404,12 @@ static void free_sig_data(struct auth_context *ctx) > * * 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 fmp_payload_header_params *fmp_ph_params, > - uint64_t mcount, char *privkey_file, char *cert_file, > +static int create_fwbin(const char *path, const char *bin, > + const efi_guid_t *guid, unsigned long index, > + unsigned long instance, > + const struct fmp_payload_header_params *fmp_ph_params, > + uint64_t mcount, > + const char *privkey_file, const char *cert_file, > uint16_t oemflags) Why shouldn't instance, mcount, oemflags be constant? > { > struct efi_capsule_header header; > @@ -604,7 +608,8 @@ void convert_uuid_to_guid(unsigned char *buf) > buf[7] = c; > } > > -static int create_empty_capsule(char *path, efi_guid_t *guid, bool fw_accept) > +static int create_empty_capsule(const char *path, const efi_guid_t *guid, > + bool fw_accept) Why should fw_accept not be constant? Please, make the use of 'const' a bit more consistent. Best regards Heinrich > { > struct efi_capsule_header header = { 0 }; > FILE *f = NULL; > @@ -666,7 +671,7 @@ int main(int argc, char **argv) > unsigned long index, instance; > uint64_t mcount; > unsigned long oemflags; > - char *privkey_file, *cert_file; > + const char *privkey_file, *cert_file; > int c, idx; > struct fmp_payload_header_params fmp_ph_params = { 0 }; >
Hello Heinrich, thank you for you extensive review. I will incorporate your reviews in a future version of the patch series. Best Regards Malte Am 16.06.2023 um 20:18 schrieb Heinrich Schuchardt: > On 6/16/23 13:34, Stefan Herbrechtsmeier wrote: >> From: Malte Schmidt <malte.schmidt@weidmueller.com> > > Thanks for considering which parameters may be constants. > > nits: > > The Urban Dictionary defines 'constify' as: > > "To constantly do something, like constantly watching anime all day." > > %s/constify function parameters/make function parameters const/ > >> >> Use const keyword for function parameters where appropriate. >> >> Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com> >> Signed-off-by: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.com> >> --- >> >> tools/mkeficapsule.c | 27 ++++++++++++++++----------- >> 1 file changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c >> index 52be1f122e..b8db00b16b 100644 >> --- a/tools/mkeficapsule.c >> +++ b/tools/mkeficapsule.c >> @@ -88,8 +88,8 @@ static void print_usage(void) >> * are filled in by create_auth_data(). >> */ >> struct auth_context { >> - char *key_file; >> - char *cert_file; >> + const char *key_file; >> + const char *cert_file; >> uint8_t *image_data; >> size_t image_size; >> struct efi_firmware_image_authentication auth; >> @@ -112,7 +112,7 @@ static int dump_sig; >> * * 0 - on success >> * * -1 - on failure >> */ >> -static int read_bin_file(char *bin, uint8_t **data, off_t *bin_size) >> +static int read_bin_file(const char *bin, uint8_t **data, off_t >> *bin_size) >> { >> FILE *g; >> struct stat bin_stat; >> @@ -170,7 +170,8 @@ err: >> * * 0 - on success >> * * -1 - on failure >> */ >> -static int write_capsule_file(FILE *f, void *data, size_t size, >> const char *msg) >> +static int write_capsule_file(FILE *f, const void *data, size_t size, > > Why should size not be constant? > > The name of the parameter 'size' does not match the function > documentation which has 'bin_size'. Please, change either of both. > > Parameter 'f' does not match the documentation which has 'bin'. > > For each function that you touch, please, ensure that the function > parameters are correctly documented. > >> + const char *msg) >> { >> size_t size_written; >> >> @@ -343,7 +344,8 @@ static int create_auth_data(struct auth_context >> *ctx) >> * * 0 - on success >> * * -1 - on failure >> */ >> -static int dump_signature(const char *path, uint8_t *signature, >> size_t sig_size) >> +static int dump_signature(const char *path, const uint8_t *signature, >> + size_t sig_size) > > Why should sig_size not be constant? > >> { >> char *sig_path; >> FILE *f; >> @@ -402,10 +404,12 @@ static void free_sig_data(struct auth_context >> *ctx) >> * * 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 fmp_payload_header_params *fmp_ph_params, >> - uint64_t mcount, char *privkey_file, char *cert_file, >> +static int create_fwbin(const char *path, const char *bin, >> + const efi_guid_t *guid, unsigned long index, >> + unsigned long instance, >> + const struct fmp_payload_header_params *fmp_ph_params, >> + uint64_t mcount, >> + const char *privkey_file, const char *cert_file, >> uint16_t oemflags) > > Why shouldn't instance, mcount, oemflags be constant? > >> { >> struct efi_capsule_header header; >> @@ -604,7 +608,8 @@ void convert_uuid_to_guid(unsigned char *buf) >> buf[7] = c; >> } >> >> -static int create_empty_capsule(char *path, efi_guid_t *guid, bool >> fw_accept) >> +static int create_empty_capsule(const char *path, const efi_guid_t >> *guid, >> + bool fw_accept) > > Why should fw_accept not be constant? > > Please, make the use of 'const' a bit more consistent. > > Best regards > > Heinrich > >> { >> struct efi_capsule_header header = { 0 }; >> FILE *f = NULL; >> @@ -666,7 +671,7 @@ int main(int argc, char **argv) >> unsigned long index, instance; >> uint64_t mcount; >> unsigned long oemflags; >> - char *privkey_file, *cert_file; >> + const char *privkey_file, *cert_file; >> int c, idx; >> struct fmp_payload_header_params fmp_ph_params = { 0 }; >> >
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 52be1f122e..b8db00b16b 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -88,8 +88,8 @@ static void print_usage(void) * are filled in by create_auth_data(). */ struct auth_context { - char *key_file; - char *cert_file; + const char *key_file; + const char *cert_file; uint8_t *image_data; size_t image_size; struct efi_firmware_image_authentication auth; @@ -112,7 +112,7 @@ static int dump_sig; * * 0 - on success * * -1 - on failure */ -static int read_bin_file(char *bin, uint8_t **data, off_t *bin_size) +static int read_bin_file(const char *bin, uint8_t **data, off_t *bin_size) { FILE *g; struct stat bin_stat; @@ -170,7 +170,8 @@ err: * * 0 - on success * * -1 - on failure */ -static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg) +static int write_capsule_file(FILE *f, const void *data, size_t size, + const char *msg) { size_t size_written; @@ -343,7 +344,8 @@ static int create_auth_data(struct auth_context *ctx) * * 0 - on success * * -1 - on failure */ -static int dump_signature(const char *path, uint8_t *signature, size_t sig_size) +static int dump_signature(const char *path, const uint8_t *signature, + size_t sig_size) { char *sig_path; FILE *f; @@ -402,10 +404,12 @@ static void free_sig_data(struct auth_context *ctx) * * 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 fmp_payload_header_params *fmp_ph_params, - uint64_t mcount, char *privkey_file, char *cert_file, +static int create_fwbin(const char *path, const char *bin, + const efi_guid_t *guid, unsigned long index, + unsigned long instance, + const struct fmp_payload_header_params *fmp_ph_params, + uint64_t mcount, + const char *privkey_file, const char *cert_file, uint16_t oemflags) { struct efi_capsule_header header; @@ -604,7 +608,8 @@ void convert_uuid_to_guid(unsigned char *buf) buf[7] = c; } -static int create_empty_capsule(char *path, efi_guid_t *guid, bool fw_accept) +static int create_empty_capsule(const char *path, const efi_guid_t *guid, + bool fw_accept) { struct efi_capsule_header header = { 0 }; FILE *f = NULL; @@ -666,7 +671,7 @@ int main(int argc, char **argv) unsigned long index, instance; uint64_t mcount; unsigned long oemflags; - char *privkey_file, *cert_file; + const char *privkey_file, *cert_file; int c, idx; struct fmp_payload_header_params fmp_ph_params = { 0 };