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

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 15.12.2013 13:41, Andrey Borzenkov wrote:
> В 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.
> 
I've tested on windows (as applied in phcoder/windows64 branch). It's
fast enough as well. So go ahead for all 3 hunks.
>>                       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.
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

signature.asc
Description: OpenPGP digital signature