Re: [PATCH v5 4/5] normal/main: Search for specific config files for netboot

2019-10-30 Thread Javier Martinez Canillas 
Hello Vladimir,

On 10/29/19 5:18 PM, Vladimir 'phcoder' Serbinenko wrote:
> What is the problem with main grub.cfg just including the files based on
> environment variables?
>

Do you mean having a grub.cfg that fetches the machine specific grub.cfg? i.e:

configfile (tftp,$net_default_server)/EFI/BOOT/grub.cfg.$net_default_ip

While is true that this could be done, is not ideal since then all the logic
to determine which config file to fetch will have to be encoded into this first
stage grub.cfg that will be downloaded by all the machines.

This policy will have to be written as a GRUB config script instead of being in
the installation server backend, that could just provide the machine specific
config files using the suffixes convention introduced by this patch-set.

And when managing a big fleet of machines the monolithic approach of having all
this policy in a single grub.cfg file could be hard to maintain and error prone.

As mentioned in the commit message, others bootloaders already support this. For
example pxelinux and yaboot. So users migrating from pxelinux and yaboot to GRUB
for PXE booting asked for this since they already have built infrastructure that
rely on this feature.

We have been carrying these patches since 2012 and our users expect this feature
to be available for their network installation environments. I also think that's
useful and gives GRUB feature parity with other bootloaders for network booting.

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


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v3 2/2] Make editenv chase symlinks including those across devices

2019-10-30 Thread Javier Martinez Canillas 
From: Peter Jones 

The grub-editenv create command will wrongly overwrite /boot/grub2/grubenv
with a regular file if grubenv is a symbolic link. But instead, it should
create a new file in the path the symlink points to.

This lets /boot/grub2/grubenv be a symlink to /boot/efi/EFI/fedora/grubenv
even when they're different mount points, which allows grub2-editenv to be
the same across platforms (i.e. UEFI vs BIOS).

For example, in Fedora the GRUB EFI builds have prefix set to /EFI/fedora
(on the EFI System Partition), but for BIOS machine it'll be /boot/grub2
(which may or may not be its own mountpoint).

With this patch, on EFI machines we can make /boot/grub2/grubenv a symlink
to /boot/efi/EFI/fedora/grubenv, and the same copy of grub-set-default will
work on both kinds of systems.

Signed-off-by: Peter Jones 
Signed-off-by: Jonathan Lebon 
Reviewed-by: Adam Jackson 
Signed-off-by: Javier Martinez Canillas 

---
Daniel,

I didn't add a check for x* allocation functions since as mentioned in my
previous email these are not needed. The functions already check for NULL
and call exit(3) if that's the case.

Changes in v3:
- A couple of style cleanups.

Changes in v2:
- Address issues pointed out by Daniel Kiper in the previous version.

 Makefile.util.def | 11 +
 util/editenv.c| 59 +--
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git Makefile.util.def Makefile.util.def
index 969d32f0097..733a397cb2b 100644
--- Makefile.util.def
+++ 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)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
diff --git util/editenv.c util/editenv.c
index eb2d0c03a98..ce00da3fc1b 100644
--- util/editenv.c
+++ util/editenv.c
@@ -28,15 +28,19 @@
 
 #include 
 #include 
+#include 
 
 #define DEFAULT_ENVBLK_SIZE1024
 
 void
 grub_util_create_envblk_file (const char *name)
 {
+  int rc;
   FILE *fp;
   char *buf;
   char *namenew;
+  ssize_t size = 1;
+  char *rename_target = xstrdup (name);
 
   buf = xmalloc (DEFAULT_ENVBLK_SIZE);
 
@@ -60,7 +64,58 @@ 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);
+  while (1)
+{
+  char *linkbuf;
+  ssize_t retsize;
+
+  linkbuf = xmalloc (size + 1);
+  retsize = grub_util_readlink (rename_target, linkbuf, size);
+  if (retsize < 0 && (errno == ENOENT || errno == EINVAL))
+{
+  free (linkbuf);
+  break;
+}
+  else if (retsize < 0)
+{
+  free (linkbuf);
+  grub_util_error (_("cannot rename the file %s to %s: %m"), namenew, 
name);
+}
+  else if (retsize == size)
+{
+  free (linkbuf);
+  size += 128;
+  continue;
+}
+
+  linkbuf[retsize] = '\0';
+  if (linkbuf[0] == '/')
+{
+  free (rename_target);
+  rename_target = linkbuf;
+}
+  else
+{
+  char *dbuf = xstrdup (rename_target);
+  const char *dir = dirname (dbuf);
+
+  free (rename_target);
+  rename_target = xasprintf ("%s/%s", dir, linkbuf);
+  free (dbuf);
+  free (linkbuf);
+}
+}
+
+  rc = grub_util_rename (namenew, rename_target);
+  if (rc < 0 && errno == EXDEV)
+{
+  rc = grub_install_copy_file (namenew, rename_target, 1);
+  grub_util_unlink (namenew);
+}
+
+  if (rc < 0)
+grub_util_error (_("cannot rename the file %s to %s: %m"), namenew, name);
+
   free (namenew);
+  free (rename_target);
 }
-- 
2.21.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v3 1/2] Add grub_util_readlink()

2019-10-30 Thread Javier Martinez Canillas 
From: Peter Jones 

Currently grub-editenv and related tools are not able to follow symbolic
links when finding their config file. For example the grub-editenv create
command will wrongly overwrite a symlink in /boot/grub2/grubenv with a new
regular file, instead of creating a file in the path the symlink points to.

A following patch will change that and add support in grub-editenv to
follow symbolic links when finding the grub environment variables file.

Add a grub_util_readlink() helper function that is just a wrapper around
the platform specific function to read the value of a symbolic link. This
helper function will be used by the following patch for grub-editenv.

Signed-off-by: Peter Jones 
Reviewed-by: Adam Jackson 
Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Daniel Kiper 

---

Changes in v3: None
Changes in v2:
- Add a better commit message explaining why the change is needed.
- Add Reviewed-by tag from Daniel Kiper.

 grub-core/osdep/windows/hostdisk.c| 6 ++
 include/grub/osdep/hostfile_aros.h| 6 ++
 include/grub/osdep/hostfile_unix.h| 6 ++
 include/grub/osdep/hostfile_windows.h | 2 ++
 4 files changed, 20 insertions(+)

diff --git grub-core/osdep/windows/hostdisk.c grub-core/osdep/windows/hostdisk.c
index 355100789a7..87a106c9b82 100644
--- grub-core/osdep/windows/hostdisk.c
+++ grub-core/osdep/windows/hostdisk.c
@@ -365,6 +365,12 @@ grub_util_mkdir (const char *dir)
   free (windows_name);
 }
 
+ssize_t
+grub_util_readlink (const char *name, char *buf, size_t bufsize)
+{
+  return readlink(name, buf, bufsize);
+}
+
 int
 grub_util_rename (const char *from, const char *to)
 {
diff --git include/grub/osdep/hostfile_aros.h include/grub/osdep/hostfile_aros.h
index a059c0fa40a..161fbb7bdfd 100644
--- include/grub/osdep/hostfile_aros.h
+++ include/grub/osdep/hostfile_aros.h
@@ -68,6 +68,12 @@ grub_util_rename (const char *from, const char *to)
   return rename (from, to);
 }
 
+static inline ssize_t
+grub_util_readlink (const char *name, char *buf, size_t bufsize)
+{
+  return readlink(name, buf, bufsize);
+}
+
 #define grub_util_mkdir(a) mkdir ((a), 0755)
 
 struct grub_util_fd
diff --git include/grub/osdep/hostfile_unix.h include/grub/osdep/hostfile_unix.h
index 9ffe46fa3ca..17cd3aa8b30 100644
--- include/grub/osdep/hostfile_unix.h
+++ include/grub/osdep/hostfile_unix.h
@@ -71,6 +71,12 @@ grub_util_rename (const char *from, const char *to)
   return rename (from, to);
 }
 
+static inline ssize_t
+grub_util_readlink (const char *name, char *buf, size_t bufsize)
+{
+  return readlink(name, buf, bufsize);
+}
+
 #define grub_util_mkdir(a) mkdir ((a), 0755)
 
 #if defined (__NetBSD__)
diff --git include/grub/osdep/hostfile_windows.h 
include/grub/osdep/hostfile_windows.h
index bf6451b6db4..8c92d0591bb 100644
--- include/grub/osdep/hostfile_windows.h
+++ include/grub/osdep/hostfile_windows.h
@@ -41,6 +41,8 @@ typedef struct grub_util_fd_dir *grub_util_fd_dir_t;
 
 int
 grub_util_rename (const char *from, const char *to);
+ssize_t
+grub_util_readlink (const char *name, char *buf, size_t bufsize);
 int
 grub_util_unlink (const char *name);
 void
-- 
2.21.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

2019-10-30 Thread Daniel Kiper 
On Wed, Oct 30, 2019 at 11:48:14AM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones 
>
> The grub-editenv create command will wrongly overwrite /boot/grub2/grubenv
> with a regular file if grubenv is a symbolic link. But instead, it should
> create a new file in the path the symlink points to.
>
> This lets /boot/grub2/grubenv be a symlink to /boot/efi/EFI/fedora/grubenv
> even when they're different mount points, which allows grub2-editenv to be
> the same across platforms (i.e. UEFI vs BIOS).
>
> For example, in Fedora the GRUB EFI builds have prefix set to /EFI/fedora
> (on the EFI System Partition), but for BIOS machine it'll be /boot/grub2
> (which may or may not be its own mountpoint).
>
> With this patch, on EFI machines we can make /boot/grub2/grubenv a symlink
> to /boot/efi/EFI/fedora/grubenv, and the same copy of grub-set-default will
> work on both kinds of systems.
>
> Signed-off-by: Peter Jones 
> Signed-off-by: Jonathan Lebon 
> Reviewed-by: Adam Jackson 
> Signed-off-by: Javier Martinez Canillas 

Reviewed-by: Daniel Kiper 

> ---
> Daniel,
>
> I didn't add a check for x* allocation functions since as mentioned in my
> previous email these are not needed. The functions already check for NULL
> and call exit(3) if that's the case.

Argh... This reminds me that I was asking somebody else about that and
got the same reply. So, I have to get used to the x* allocation
functions behavior. Sorry for the confusion.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable pager by default

2019-10-30 Thread Daniel Kiper 
On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> On 10/24/19 4:50 PM, Daniel Kiper wrote:
>
> [snip]
>
> >>
> >> From 7c4da6295ebd3a034d1f7e32099eab33efa465d4 Mon Sep 17 00:00:00 2001
> >> From: Javier Martinez Canillas 
> >> Date: Tue, 22 Oct 2019 15:35:12 +0200
> >> Subject: [PATCH v2] Add a GRUB_COMMAND_FLAG_PAGINATED to request paginated
> >>  output for commands
> >>
> >> When user enters into the GRUB shell and tries to use help command, lot of
> >> information is scrolled out of screen and the user doesn't have chance to
> >> read it. Also, there isn't any information about 'set pager=1' at the end
> >> of the help output, to tell the user how scrolling could be enabled.
> >>
> >> Since the out for some commands may not fit into a screen, add a new flag
> >> GRUB_COMMAND_FLAG_PAGINATED that can be used when registering commands that
> >> can be used to request the pager to be enabled while executing the handler.
> >>
> >> Signed-off-by: Javier Martinez Canillas 
> >
> > In general I like the idea but I think patch requires some polishing...
> >
> > Hmmm... Still thinking about "-p" flag which allows user to choose
> > between pager on/off. Or something which I proposed in the email to
> > Michael...
>
> I'm OK with any of the other two approaches too. But it seems that Vladimir
> is worried about the added complexity for these.
>
> I honestly think that the approach in this patch is the least bad option since
> it doesn't require a special configuration or the user to do anything, just
> execute the command printing a lot of stuff and get paginated output by 
> default.
>
> Can't think why users would want to execute commands that print a lot of 
> messages
> without the output being paginated, or why they would want to execute a 
> command
> like help in in batch mode. It's only useful in interactive mode.
>
> And also the patch is quite trivial, it already uses all the existing code for
> the pager option.
>
> But I'll wait for the discussion to settle and a solution to be agreed upon,
> before posting a new patch for this.

After some thinking it seems to me that Valdimir is right and we should
not over-complicate the solution. Let's print the message on interactive
screen how to enable paging.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable pager by default

2019-10-30 Thread adrian15 adrian15 
I agree.

One of the Super Grub2 Disk options consists on showing from its menu the
COPYRIGHT page... the pager variable is set to true so that the output is
paged when the cat command is run.
This usecase might not work anymore if something else than prompting a
message on the interactive screen was used.

El mié., 30 oct. 2019 a las 13:12, Daniel Kiper ()
escribió:

> On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:
> > Hello Daniel,
> >
> > On 10/24/19 4:50 PM, Daniel Kiper wrote:
> >
> > [snip]
> >
> > >>
> > >> From 7c4da6295ebd3a034d1f7e32099eab33efa465d4 Mon Sep 17 00:00:00 2001
> > >> From: Javier Martinez Canillas 
> > >> Date: Tue, 22 Oct 2019 15:35:12 +0200
> > >> Subject: [PATCH v2] Add a GRUB_COMMAND_FLAG_PAGINATED to request
> paginated
> > >>  output for commands
> > >>
> > >> When user enters into the GRUB shell and tries to use help command,
> lot of
> > >> information is scrolled out of screen and the user doesn't have
> chance to
> > >> read it. Also, there isn't any information about 'set pager=1' at the
> end
> > >> of the help output, to tell the user how scrolling could be enabled.
> > >>
> > >> Since the out for some commands may not fit into a screen, add a new
> flag
> > >> GRUB_COMMAND_FLAG_PAGINATED that can be used when registering
> commands that
> > >> can be used to request the pager to be enabled while executing the
> handler.
> > >>
> > >> Signed-off-by: Javier Martinez Canillas 
> > >
> > > In general I like the idea but I think patch requires some polishing...
> > >
> > > Hmmm... Still thinking about "-p" flag which allows user to choose
> > > between pager on/off. Or something which I proposed in the email to
> > > Michael...
> >
> > I'm OK with any of the other two approaches too. But it seems that
> Vladimir
> > is worried about the added complexity for these.
> >
> > I honestly think that the approach in this patch is the least bad option
> since
> > it doesn't require a special configuration or the user to do anything,
> just
> > execute the command printing a lot of stuff and get paginated output by
> default.
> >
> > Can't think why users would want to execute commands that print a lot of
> messages
> > without the output being paginated, or why they would want to execute a
> command
> > like help in in batch mode. It's only useful in interactive mode.
> >
> > And also the patch is quite trivial, it already uses all the existing
> code for
> > the pager option.
> >
> > But I'll wait for the discussion to settle and a solution to be agreed
> upon,
> > before posting a new patch for this.
>
> After some thinking it seems to me that Valdimir is right and we should
> not over-complicate the solution. Let's print the message on interactive
> screen how to enable paging.
>
> Daniel
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Support free software. Donate to Super Grub Disk. Apoya el software libre.
Dona a Super Grub Disk. https://www.supergrubdisk.org/donate/
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable pager by default

2019-10-30 Thread Javier Martinez Canillas 
On 10/30/19 1:12 PM, Daniel Kiper wrote:
> On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:

[snip]

>>
>> But I'll wait for the discussion to settle and a solution to be agreed upon,
>> before posting a new patch for this.
> 
> After some thinking it seems to me that Valdimir is right and we should
> not over-complicate the solution. Let's print the message on interactive
> screen how to enable paging.
>

Ok, sounds good to me.
 
> Daniel
> 

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

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v5 4/5] normal/main: Search for specific config files for netboot

2019-10-30 Thread Vladimir 'phcoder' Serbinenko 
We had a discussion with Daniel and agreed on following:
1) we accept the feature
2) we want to have an option to specify grub config file. And if this
option is present, the scanning would be disabled
3) to avoid multiple round-trips can we request all the possible files in
parallel?

On Wed, 30 Oct 2019, 11:40 Javier Martinez Canillas, 
wrote:

> Hello Vladimir,
>
> On 10/29/19 5:18 PM, Vladimir 'phcoder' Serbinenko wrote:
> > What is the problem with main grub.cfg just including the files based on
> > environment variables?
> >
>
> Do you mean having a grub.cfg that fetches the machine specific grub.cfg?
> i.e:
>
> configfile (tftp,$net_default_server)/EFI/BOOT/grub.cfg.$net_default_ip
>
> While is true that this could be done, is not ideal since then all the
> logic
> to determine which config file to fetch will have to be encoded into this
> first
> stage grub.cfg that will be downloaded by all the machines.
>
> This policy will have to be written as a GRUB config script instead of
> being in
> the installation server backend, that could just provide the machine
> specific
> config files using the suffixes convention introduced by this patch-set.
>
> And when managing a big fleet of machines the monolithic approach of
> having all
> this policy in a single grub.cfg file could be hard to maintain and error
> prone.
>
> As mentioned in the commit message, others bootloaders already support
> this. For
> example pxelinux and yaboot. So users migrating from pxelinux and yaboot
> to GRUB
> for PXE booting asked for this since they already have built
> infrastructure that
> rely on this feature.
>
> We have been carrying these patches since 2012 and our users expect this
> feature
> to be available for their network installation environments. I also think
> that's
> useful and gives GRUB feature parity with other bootloaders for network
> booting.
>
> Best regards,
> --
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat
>
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v5 4/5] normal/main: Search for specific config files for netboot

2019-10-30 Thread Javier Martinez Canillas 
On 10/30/19 2:45 PM, Vladimir 'phcoder' Serbinenko wrote:
> We had a discussion with Daniel and agreed on following:
> 1) we accept the feature

Great, thanks.

> 2) we want to have an option to specify grub config file. And if this
> option is present, the scanning would be disabled

Sure, you mentioned that this should be a DHCP option. Which option were
you thinking that could be used for this?

> 3) to avoid multiple round-trips can we request all the possible files in
> parallel?
> 

It this even possible with GRUB? I thought that didn't have multi-tasking
support so we couldn't parallelize the requests to the TFTP server.

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


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel