Re: [PATCH 2/2] Make editenv chase symlinks including those across devices.

[Javier Martinez Canillas]

On 10/23/19 11:42 AM, Daniel Kiper wrote:
> On Fri, Oct 18, 2019 at 02:42:21PM +0200, Javier Martinez Canillas wrote:
>> From: Peter Jones <address@hidden>
>>
>> This lets us make /boot/grub2/grubenv a symlink to
> 
> Who or what creates that link?
>

That symlink is created by the Fedora grub2-efi package. On legacy BIOS
installs is just a regular file from the grub2-pc package. That way for
both EFI and legacy BIOS installs the /boot/grub2/grubenv is always used,
even when for EFI the grubenv is in the ESP.

In both cases is the installer (Anaconda) the component that creates and
sets the values of the actual grubenv file.

>> /boot/efi/EFI/fedora/grubenv even though they're different mount points,
>> which allows /usr/bin/grub2-editenv to be the same across platforms
>> (i.e. UEFI vs BIOS).
>>
>> Signed-off-by: Peter Jones <address@hidden>
>> Reviewed-by: Adam Jackson <address@hidden>
>> Signed-off-by: Javier Martinez Canillas <address@hidden>
>> ---
>>
>>  Makefile.util.def | 11 +++++++++++
>>  util/editenv.c    | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile.util.def b/Makefile.util.def
>> index 969d32f0097..733a397cb2b 100644
>> --- a/Makefile.util.def
>> +++ b/Makefile.util.def
>> @@ -240,8 +240,19 @@ program = {
>>
>>    common = util/grub-editenv.c;
>>    common = util/editenv.c;
>> +  common = util/grub-install-common.c;
>>    common = grub-core/osdep/init.c;
>> +  common = grub-core/osdep/compress.c;
>> +  extra_dist = grub-core/osdep/unix/compress.c;
>> +  extra_dist = grub-core/osdep/basic/compress.c;
>> +  common = util/mkimage.c;
>> +  common = util/grub-mkimage32.c;
>> +  common = util/grub-mkimage64.c;
>> +  common = grub-core/osdep/config.c;
>> +  common = util/config.c;
>> +  common = util/resolve.c;
>>
>> +  ldadd = '$(LIBLZMA)';
> 
> Hmmm... Why do we need that change? Does not it belong to different patch?
>

That's needed due a dependency. The patch uses grub_install_copy_file() from
util/grub-install-common.c and that uses grub_install_generate_image() that
is defined in util/mkimage.c, which in turn uses lzma_stream_encoder().

So all the files added to common for grub-editenv are due dependencies pulled
by grub-editenv now using the grub_install_copy_file() function.

>>    ldadd = libgrubmods.a;
>>    ldadd = libgrubgcry.a;
>>    ldadd = libgrubkern.a;
>> diff --git a/util/editenv.c b/util/editenv.c
>> index eb2d0c03a98..e61dc1283a4 100644
>> --- a/util/editenv.c
>> +++ b/util/editenv.c
>> @@ -37,6 +37,7 @@ grub_util_create_envblk_file (const char *name)
>>    FILE *fp;
>>    char *buf;
>>    char *namenew;
>> +  char *rename_target = xstrdup(name);
>>
>>    buf = xmalloc (DEFAULT_ENVBLK_SIZE);
>>
>> @@ -60,7 +61,48 @@ grub_util_create_envblk_file (const char *name)
>>    free (buf);
>>    fclose (fp);
>>
>> -  if (grub_util_rename (namenew, name) < 0)
>> -    grub_util_error (_("cannot rename the file %s to %s"), namenew, name);
>> +  ssize_t size = 1;
> 
> I do not like variables declarations mixed with the code in that way.
> Please move it to the beginning of the function.
>

Ok.

>> +  while (1)
>> +    {
>> +      char *linkbuf;
>> +      ssize_t retsize;
> 
> ...but I am OK with that... Hmmm... And it seems to me that size
> can be moved here.
>

It can't be moved inside the while loop because the initial condition has
to be set outside the loop. I'll move it at the beginning of the function
as you suggested.

>> +
>> +      linkbuf = xmalloc(size+1);
> 
> linkbuf = xmalloc (size + 1); Missing spaces...
>

Ok.

>> +      retsize = grub_util_readlink (rename_target, linkbuf, size);
>> +      if (retsize < 0 && (errno == ENOENT || errno == EINVAL))
>> +    {
>> +      free (linkbuf);
>> +      break;
>> +    }
>> +      else if (retsize < 0)
>> +    {
>> +      grub_util_error (_("cannot rename the file %s to %s: %m"), namenew, 
>> name);
>> +      free (linkbuf);
>> +      free (namenew);
>> +      return;
>> +    }
>> +      else if (retsize == size)
>> +    {
>> +      free(linkbuf);
> 
> free (linkbuf);
>

Ok.

>> +      size += 128;
>> +      continue;
>> +    }
>> +
>> +      free (rename_target);
>> +      linkbuf[retsize] = '\0';
>> +      rename_target = linkbuf;
>> +    }
>> +
>> +  int rc = grub_util_rename (namenew, rename_target);
> 
> Please move this variable declaration to the beginning of function.
>

Ok.

> Daniel
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat