Thanks for your review, I'll rework my patch according to your comments.
I just kept the open points in this thread.
Le 09/09/2021 à 17:46, Daniel Kiper a écrit :
+static grub_err_t grub_efi_set_watchdog_timer(unsigned long timeout)
Please use grub_efi_uintn_t instead of unsigned long. However, please
remember that its size depends on architecture. So, it can be 32-bits or
64-bits...
I'll follow the uefi spec here so grub_efi_uintn_ is fine.
+ The numeric code to log on a watchdog timer timeout event.
+ The firmware reserves codes 0x0000 to 0xFFFF.
+ Loaders and operating systems may use other timeout codes.
This comment says you cannot use 0xb007. I would use 0xdeadb007deadb007
by default. However, I would give an option to change the default by user.
Stupid me ....
I was wondering if the code should be configurable.
In the initial patch it was the case but though this would give an
easier command line interface to users.
I mean, the aim of this code is to report mostly 'who' manipulated the
watchdog, so sound good to me to get a value that is more grub depend
than user centric.
+ */
Please format comments according to [2].
+ grub_efi_uint64_t code = 0xB007;
+ grub_efi_status_t status = 0;
+
+ if (timeout >= 0xFFFF)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("efi-watchdog: timeout must be
lower than 65535"));
Why do you artificially limit the timeout value? Please do not do that.
Mistake of my own, make sense to remove that.
+ if (timeout > 0)
+ grub_printf("grub %s: Arming EFI watchdog at %lu seconds\n",
PACKAGE_VERSION, timeout);
If you want debug messages please use grub_dprintf() instead of grub_printf().
Maybe my print isn't ideal but I found important to warn the user a
watchdog is armed.
If the user is not informed and the watchdog fires, the user could think
there is a hardware issue on the host.
So to me, this message is mandatory to say users : "beware, the system
will be reset within x seconds if you don't boot"
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel