Re: [PATCH] trivial fixes for windows-efi branch

[Andrey Borzenkov]

В Sun, 15 Dec 2013 12:43:49 +0100
"Vladimir 'phcoder' Serbinenko" <address@hidden> пишет:

> Sounds like you're looking at old version. I've fixed those problems and
> it's already merged in master. Could you have a look and say if any of
> those still a problem?

Yes, it fixed file path and variable not existed in exhaustive search.
Still see comments below.

diff --git a/grub-core/osdep/windows/platform.c
b/grub-core/osdep/windows/platform.c index b123256..3f4ad5e 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev, void *current = NULL;
          ssize_t current_len;
          current = get_efi_variable_bootn (i, &current_len);
+         if (current_len < 0)
+           continue; /* FIXME Should we abort on error? */
          if (current_len < (distrib16_len + 1) * sizeof
(grub_uint16_t)
              + 6)
            {

Same potential problem with signed vs. unsigned comparison. Variable is
not guaranteed to exist here and -1 is treated as very large unsigned
value.

@@ -275,13 +277,18 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev, void *current = NULL;
          ssize_t current_len;
          current = get_efi_variable_bootn (i, &current_len);
+         if (current_len < -1)
+           continue; /* FIXME Should we abort on error? */

For completeness we should handle failures to avoid crash.

          if (current_len == -1)
            {
-             order_num = i;
-             have_order_num = 1;
-             grub_util_info ("Creating new entry at Boot%04x",
-                             order_num);
-             break;
+             if (!have_order_num)
+               {
+                 order_num = i;
+                 have_order_num = 1;
+                 grub_util_info ("Creating new entry at Boot%04x",
+                                 order_num);
+               }
+             continue;
            }

You changed it to stop on first non-existing variable; but this can
miss another one later with the same description. This will result in
two different boot entries with the same name.

I cannot test it on Windows; on Linux code appears to be fast enough.

>                       As for merging with Linux part we could just have
> primitives set variable and get variable and have everything else shared

Yes, I'll get a look.