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.
> 
Yes, apparently mingw treats here as siged comparison and no warning is
emitted. As for the comment, the reasons for possible failure are
somewhat unclear to me, other than permission denied one. On Windows you
wouldn't get here if permission is denied (you'd stop at token
manipulation). Some kind of internal EFI failure may leave some
variables unusable while otherwise allowing algorithm to work.
> @@ -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.
> 
Same.
Go ahead for those 2 hunks.
>         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'll test think hunk on my next reboot.
>>                       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