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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

On 15.12.2013 13:41, Andrey Borzenkov wrote:
> В Sun, 15 Dec 2013 12:43:49 +0100
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> 
>> 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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]