From: Daniel Kiper <dkiper@net-space.pl>
To: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
Cc: The development of GRUB 2 <grub-devel@gnu.org>
Subject: Re: [PATCH 1/4] efi: Deduplicate configuration table search function
Date: Tue, 17 Oct 2023 16:38:20 +0200 [thread overview]
Message-ID: <20231017143820.htk6b3xsskyxi4j5@tomti.i.net-space.pl> (raw)
In-Reply-To: <CAEaD8JNg_jRPrBchN7kHYbQrmQJPogiNzaJnvVu_m-90hPWP+w@mail.gmail.com>
Hey,
First of all, please use "git send-email" to send patches and add cover
letter to more than one patch. Additionally, it would be nice if every
patch has commit message which explains why a given patch is needed, what
it does, etc.
Last but not least, please CC all people who were involved in the GUID
related discussions...
On Sat, Oct 07, 2023 at 04:57:28PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> From c4b2028c94cd5922461ca562f3a07eb13b6b04dd Mon Sep 17 00:00:00 2001
> From: Vladimir Serbinenko <phcoder@gmail.com>
> Date: Sun, 13 Aug 2023 09:15:02 +0200
> Subject: [PATCH 1/4] efi: Deduplicate configuration table search function
>
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
> grub-core/commands/efi/loadbios.c | 37 ++++++++-----------------------
> grub-core/commands/efi/lssal.c | 18 +++++----------
> grub-core/commands/efi/smbios.c | 30 ++-----------------------
> grub-core/kern/efi/acpi.c | 28 ++---------------------
> grub-core/kern/efi/efi.c | 18 +++++++++++++++
> grub-core/kern/efi/fdt.c | 21 +++++-------------
> include/grub/efi/efi.h | 3 +++
> 7 files changed, 46 insertions(+), 109 deletions(-)
>
> diff --git a/grub-core/commands/efi/loadbios.c b/grub-core/commands/efi/loadbios.c
> index 8f6b0ecfc..916633631 100644
> --- a/grub-core/commands/efi/loadbios.c
> +++ b/grub-core/commands/efi/loadbios.c
> @@ -92,7 +92,6 @@ lock_rom_area (void)
> static void
> fake_bios_data (int use_rom)
> {
> - unsigned i;
> void *acpi, *smbios;
> grub_uint16_t *ebda_seg_ptr, *low_mem_ptr;
>
> @@ -101,33 +100,15 @@ fake_bios_data (int use_rom)
> if ((*ebda_seg_ptr) || (*low_mem_ptr))
> return;
>
> - acpi = 0;
> - smbios = 0;
> - for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> - {
> - grub_guid_t *guid =
> - &grub_efi_system_table->configuration_table[i].vendor_guid;
> -
> - if (! grub_memcmp (guid, &acpi2_guid, sizeof (grub_guid_t)))
> - {
> - acpi = grub_efi_system_table->configuration_table[i].vendor_table;
> - grub_dprintf ("efi", "ACPI2: %p\n", acpi);
> - }
> - else if (! grub_memcmp (guid, &acpi_guid, sizeof (grub_guid_t)))
> - {
> - void *t;
> -
> - t = grub_efi_system_table->configuration_table[i].vendor_table;
> - if (! acpi)
> - acpi = t;
> - grub_dprintf ("efi", "ACPI: %p\n", t);
> - }
> - else if (! grub_memcmp (guid, &smbios_guid, sizeof (grub_guid_t)))
> - {
> - smbios = grub_efi_system_table->configuration_table[i].vendor_table;
> - grub_dprintf ("efi", "SMBIOS: %p\n", smbios);
> - }
> - }
> + acpi = grub_efi_find_configuration_table(&acpi2_guid);
Could you stick to the GRUB coding convention?
> + grub_dprintf ("efi", "ACPI2: %p\n", acpi);
> + if (!acpi) {
> + acpi = grub_efi_find_configuration_table(&acpi_guid);
> + grub_dprintf ("efi", "ACPI1: %p\n", acpi);
I would not change ACPI to ACPI1...
> + }
> +
> + smbios = grub_efi_find_configuration_table(&smbios_guid);
> + grub_dprintf ("efi", "SMBIOS: %p\n", smbios);
>
> *ebda_seg_ptr = FAKE_EBDA_SEG;
> *low_mem_ptr = (FAKE_EBDA_SEG >> 6);
> diff --git a/grub-core/commands/efi/lssal.c b/grub-core/commands/efi/lssal.c
> index fd6085f1b..0e36bfcdc 100644
> --- a/grub-core/commands/efi/lssal.c
> +++ b/grub-core/commands/efi/lssal.c
> @@ -136,22 +136,16 @@ grub_cmd_lssal (struct grub_command *cmd __attribute__ ((unused)),
> int argc __attribute__ ((unused)),
> char **args __attribute__ ((unused)))
> {
> - const grub_efi_system_table_t *st = grub_efi_system_table;
> - grub_efi_configuration_table_t *t = st->configuration_table;
> - unsigned int i;
> static grub_guid_t guid = GRUB_EFI_SAL_TABLE_GUID;
> + void *table = grub_efi_find_configuration_table(&guid);
>
> - for (i = 0; i < st->num_table_entries; i++)
> + if (!table)
I think "if (table == NULL)" is better for NULL checks.
> {
> - if (grub_memcmp (&guid, &t->vendor_guid,
> - sizeof (grub_guid_t)) == 0)
> - {
> - disp_sal (t->vendor_table);
> - return GRUB_ERR_NONE;
> - }
> - t++;
> + grub_printf ("SAL not found\n");
Incorrect indention...
> + return GRUB_ERR_NONE;
> }
> - grub_printf ("SAL not found\n");
> +
> + disp_sal (table);
> return GRUB_ERR_NONE;
> }
>
> diff --git a/grub-core/commands/efi/smbios.c b/grub-core/commands/efi/smbios.c
> index d77239732..296bedf7b 100644
> --- a/grub-core/commands/efi/smbios.c
> +++ b/grub-core/commands/efi/smbios.c
> @@ -18,44 +18,18 @@
> */
>
> #include <grub/smbios.h>
> -#include <grub/misc.h>
> #include <grub/efi/efi.h>
> -#include <grub/efi/api.h>
I would mention in the commit message you are doing some code cleanups
on the occasion.
> struct grub_smbios_eps *
> grub_machine_smbios_get_eps (void)
> {
> - unsigned i;
> static grub_guid_t smbios_guid = GRUB_EFI_SMBIOS_TABLE_GUID;
> -
I would prefer if you leave this empty line.
> - for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> - {
> - grub_guid_t *guid =
> - &grub_efi_system_table->configuration_table[i].vendor_guid;
> -
> - if (! grub_memcmp (guid, &smbios_guid, sizeof (grub_guid_t)))
> - return (struct grub_smbios_eps *)
> - grub_efi_system_table->configuration_table[i].vendor_table;
> - }
> -
> - return 0;
> + return (struct grub_smbios_eps *) grub_efi_find_configuration_table(&smbios_guid);
Incorrect coding convention...
> }
>
> struct grub_smbios_eps3 *
> grub_machine_smbios_get_eps3 (void)
> {
> - unsigned i;
> static grub_guid_t smbios3_guid = GRUB_EFI_SMBIOS3_TABLE_GUID;
> -
Again, please do not drop empty line after variables definition.
> - for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> - {
> - grub_guid_t *guid =
> - &grub_efi_system_table->configuration_table[i].vendor_guid;
> -
> - if (! grub_memcmp (guid, &smbios3_guid, sizeof (grub_guid_t)))
> - return (struct grub_smbios_eps3 *)
> - grub_efi_system_table->configuration_table[i].vendor_table;
> - }
> -
> - return 0;
> + return (struct grub_smbios_eps3 *) grub_efi_find_configuration_table(&smbios3_guid);
Again, incorrect coding convention...
... and there are more similar mistakes below. Please fix all of them...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
next prev parent reply other threads:[~2023-10-17 14:38 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-07 14:57 [PATCH 1/4] efi: Deduplicate configuration table search function Vladimir 'phcoder' Serbinenko
2023-10-17 14:38 ` Daniel Kiper [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-10-31 19:22 Vladimir 'phcoder' Serbinenko
2023-10-31 21:41 ` John Paul Adrian Glaubitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231017143820.htk6b3xsskyxi4j5@tomti.i.net-space.pl \
--to=dkiper@net-space.pl \
--cc=grub-devel@gnu.org \
--cc=phcoder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).