[RFC][PATCH v1 0/4] How to add --set=VARNAME to the ls command?

[RFC][PATCH v1 0/4] How to add --set=VARNAME to the ls command?

[Denis 'GNUtoo' Carikli]

Hi,

The problem we try to solve with --set=VARNAME in ls.
=====================================================
In the GNU Boot project (a free software distribution that releases
free software boot firmware images), we provide images with (a
deblobbed) Coreboot and GRUB (run as a Coreboot payload). We use GRUB
mainly to find other configuration files like syslinux.cfg (to boot on
external medias) or grub.cfg (to boot on the (usually GNU/Linux)
distribution installed to the hard disk / SSD).

We also provide images with a SeaBIOS Coreboot payload instead, but we
plan to make the images with GRUB become the preffered way of booting
because in practice it works very well with the Coreboot Framebuffer,
and with it we only lack a way to reliabily list the devices being
present in order to be able to also find grub.cfg config files inside
filesystems present on LVM logical volumes as well.

The alternative to using GRUB as a Coreboot payload is to use SeaBIOS
instead but that doesn't work well because when SeaBIOS loads the
(usually GNU/Linux) distribution's GRUB, it results in a black screen
unless the users tweak the /etc/default/grub configuration to use the
'console' output instead of the default gfxterm, and we also want less
technical users to be able to easily use computers with GNU Boot. This
issue is probably due to SeaVGABIOS that probably doesn't fully
implement the VGA standard, so my guess is that fixing this is more
work than adding --set=VARNAME to the 'ls' command.

Our current GRUB configuration file is in our git repository[1] and it
hardcodes devices like ahciX,Y and then tries to find the grub.cfg
with (a limited) number of X,Y combination.

[1]https://git.savannah.gnu.org/cgit/gnuboot.git/tree/resources/grub/config/grub.cfg

Questions about the implementation
==================================
The patch set that follows is far from optimal:

* The 'commands/ls: add --set=VARNAME.' patch only implements
  --set=VARNAME for 'ls' without other arguments, and it returns an
  error otherwise. I'm not sure if it's the right solution but in
  another hand implementing --set=VARNAME for all the ls command would
  make the patch too big given how the implementation is done (more
  on that later).

* The patches adding --set=VARNAME 'commands/ls: add --set=VARNAME.'
  changes is not very intrusive but the later patch 'commands/ls:
  support --set for files/directories.' shows the broader issue very
  clearly: all the prints are duplicated with some 'if (varname) {
  ... }' construct.

Since here my goal is only to add '--set=VARNAME' for 'ls' without
arguments, what would be the best way to proceed?

Would a patch that doesn't cover all the 'ls' arguments be acceptable?
If not, I guess that the way to go would be to rework a bit the
printing as with the current way, there is too much duplication of
code and it also makes the code harder to follow which in turn makes
maintenance of this code harder.

In this case what kind of API would be acceptable? Should we introduce
some functions that have an argument that can select where to print?

If so would something similar to fprintf be ok? It could be used like
that 'grub_xfprintf( varname ? stdout : varname, "%s\n", "Hello
world");' and make the code more redable than with the 'commands/ls:
support --set for files/directories.' patch.

Denis 'GNUtoo' Carikli (4):
  Add grub_env_append function.
  Add command to append to existing environment variables.
  commands/ls: add --set=VARNAME.
  commands/ls: support --set for files/directories.

 grub-core/commands/ls.c  | 249 ++++++++++++++++++++++++++++++++++-----
 grub-core/kern/corecmd.c |  25 ++++
 grub-core/kern/env.c     |  38 ++++++
 include/grub/env.h       |   1 +
 4 files changed, 282 insertions(+), 31 deletions(-)

-- 
2.45.1


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

[RFC][PATCH v1 1/4] Add grub_env_append function.

[Denis 'GNUtoo' Carikli]

If the given environment variable doesn't exist, grub_env_append will
have the same effect than grub_env_set. But if the variable do exist,
using grub_env_append will append the given content to the variable
content.

This can be used to build a command that can append data to an
existing variable.

The goal here is to more easily add --set=VARNAME arguments to current
commands like it is done in the probe command for instance.

This is because in the code of some commands (like ls) GRUB start
printing information directly to the output instead of building a big
string and only printing the information when done building it.

And so having something like grub_env_append that is closer to this
behavior helps adding --set=VARNAME to various commands (like ls).

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/kern/env.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/grub/env.h   |  1 +
 2 files changed, 39 insertions(+)

diff --git a/grub-core/kern/env.c b/grub-core/kern/env.c
index 764068896..24ba42bb8 100644
--- a/grub-core/kern/env.c
+++ b/grub-core/kern/env.c
@@ -129,6 +129,44 @@ grub_env_set (const char *name, const char *val)
   return grub_errno;
 }
 
+grub_err_t
+grub_env_append (const char *name, const char *val)
+{
+  struct grub_env_var *var;
+
+  /* If the variable does already exist, append val to the variable content.  */
+  var = grub_env_find (name);
+  if (var)
+    {
+      char *old = var->value;
+      char *new;
+
+      new = grub_zalloc (grub_strlen(old) + grub_strlen(val) + 1);
+      if (!new)
+        return grub_errno;
+
+      grub_strcpy (new, old);
+      grub_strcpy (new + grub_strlen(new), val);
+
+      if (var->write_hook)
+        var->value = var->write_hook (var, new);
+      else
+        var->value = grub_strdup (new);
+
+      if (! var->value)
+        {
+          var->value = old;
+          grub_free (new);
+          return grub_errno;
+        }
+
+      grub_free (old);
+      return GRUB_ERR_NONE;
+    }
+
+  return grub_env_set (name, val);
+}
+
 const char *
 grub_env_get (const char *name)
 {
diff --git a/include/grub/env.h b/include/grub/env.h
index 6b9379a30..e62786006 100644
--- a/include/grub/env.h
+++ b/include/grub/env.h
@@ -44,6 +44,7 @@ struct grub_env_var
 };
 
 grub_err_t EXPORT_FUNC(grub_env_set) (const char *name, const char *val);
+grub_err_t EXPORT_FUNC(grub_env_append) (const char *name, const char *val);
 const char *EXPORT_FUNC(grub_env_get) (const char *name);
 bool EXPORT_FUNC(grub_env_get_bool) (const char *name, bool if_unset);
 void EXPORT_FUNC(grub_env_unset) (const char *name);
-- 
2.45.1


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

[RFC][PATCH v1 2/4] Add command to append to existing environment variables.

[Denis 'GNUtoo' Carikli]

This can be used to easily filter out the content of an environment
variable with multiple elements:

    for elm in $list ; do
        if regexp ^grub $elm ; then
           append results=" $elm"
        fi
    done

The goal is to use it to be able to be able to filter devices being
found once we add support for --set=VARNAME inside the ls command.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/kern/corecmd.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/grub-core/kern/corecmd.c b/grub-core/kern/corecmd.c
index 62d434ba9..88cf6e4d4 100644
--- a/grub-core/kern/corecmd.c
+++ b/grub-core/kern/corecmd.c
@@ -59,6 +59,28 @@ grub_core_cmd_set (struct grub_command *cmd __attribute__ ((unused)),
   return 0;
 }
 
+/* Append VALUE to ENVVAR content */
+static grub_err_t
+grub_core_cmd_append (struct grub_command *cmd __attribute__ ((unused)),
+                      int argc, char *argv[])
+{
+  char *var;
+  char *val;
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
+
+  var = argv[0];
+  val = grub_strchr (var, '=');
+  if (! val)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "not an assignment");
+
+  val[0] = 0;
+  grub_env_append (var, val + 1);
+
+  return 0;
+}
+
 static grub_err_t
 grub_core_cmd_unset (struct grub_command *cmd __attribute__ ((unused)),
 		     int argc, char *argv[])
@@ -182,6 +204,9 @@ grub_register_core_commands (void)
 			       N_("Set an environment variable."));
   if (cmd)
     cmd->flags |= GRUB_COMMAND_FLAG_EXTRACTOR;
+  grub_register_command ("append", grub_core_cmd_append,
+			 N_("[ENVVAR=VALUE]"),
+			 N_("Set an environment variable."));
   grub_register_command ("unset", grub_core_cmd_unset,
 			 N_("ENVVAR"),
 			 N_("Remove an environment variable."));
-- 
2.45.1


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

[RFC][PATCH v1 3/4] commands/ls: add --set=VARNAME.

[Denis 'GNUtoo' Carikli]

There is currently no way to get a list of devices being found inside
an environment variable.

The GNU Boot project is a boot firmware distribution that currently
ships images with a deblobbed Coreboot, GRUB, and a hand-made GRUB
configuration.

Once installed, the GRUB provided by GNU Boot is supposed to try to
find the GRUB configuration of the (usually GNU/Linux) distribution
that is installed on the computer.

To do that GNU Boot images includes a hand-made GRUB configuration
that have hardcoded devices names like md/0 or ahci0, and that loop
over that and test if grub.cfg is found in hardcoded paths like
/grub.cfg, /boot/grub.cfg, etc.

But that cannot work for LVM2 volume that have names chosen by the
user or that differ between GNU/Linux distribution.

So having a '--set=VARNAME' option in 'ls' can enable to loop over
all the device found or even build a list of LVM devices like that:

    ls --set=devices
    for device in $devices ; do
        if regexp ^lvm/ $device ; then
           append lvmvol=" $device"
        fi
    done

Then the GRUB configuration shipped by GNU Boot would simply try the
various hardcoded location of grub.cfg and boot on the first one being
found.

Right now this change only adds the ability to list devices, it
doesn't support the '-l' option.

Listing devices inside a variable also doesn't add the parenthesis
around the devices as this makes the use of the result more easy to
deal with.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/commands/ls.c | 69 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 11 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 6a1c7f5d3..113235781 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -37,31 +37,69 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 static const struct grub_arg_option options[] =
   {
+    {"set",             's', 0,
+     N_("Set a variable to return value."), N_("VARNAME"), ARG_TYPE_STRING},
     {"long", 'l', 0, N_("Show a long list with more detailed information."), 0, 0},
     {"human-readable", 'h', 0, N_("Print sizes in a human readable format."), 0, 0},
     {"all", 'a', 0, N_("List all files."), 0, 0},
     {0, 0, 0, 0, 0, 0}
   };
 
+/* Context for grub_ls_list_devices.  */
+struct grub_ls_list_devices_ctx
+{
+  int longlist;
+  char *varname;
+};
+
 /* Helper for grub_ls_list_devices.  */
 static int
 grub_ls_print_devices (const char *name, void *data)
 {
-  int *longlist = data;
+  struct grub_ls_list_devices_ctx *ctx = data;
 
-  if (*longlist)
-    grub_normal_print_device_info (name);
+  if (ctx->longlist)
+    {
+      if (ctx->varname)
+	{
+	  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "set and longlist");
+	  return GRUB_ERR_NOT_IMPLEMENTED_YET;
+	}
+
+      grub_normal_print_device_info (name);
+      return 0;
+    }
+
+  if (ctx->varname)
+    {
+      if (grub_env_get(ctx->varname))
+	grub_env_append (ctx->varname, " ");
+      grub_env_append (ctx->varname, name);
+    }
   else
-    grub_printf ("(%s) ", name);
+    {
+      grub_printf ("(%s) ", name);
+    }
 
   return 0;
 }
 
 static grub_err_t
-grub_ls_list_devices (int longlist)
+grub_ls_list_devices (int longlist, char *varname)
 {
-  grub_device_iterate (grub_ls_print_devices, &longlist);
-  grub_xputs ("\n");
+  struct grub_ls_list_devices_ctx ctx = {
+    .longlist = longlist,
+    .varname = varname,
+  };
+
+  /* Clear the variable content to be able to append inside later on */
+  if (varname)
+    grub_env_unset(varname);
+
+  grub_device_iterate (grub_ls_print_devices, &ctx);
+
+  if (!varname)
+    grub_xputs ("\n");
 
 #if 0
   {
@@ -171,13 +209,20 @@ print_files_long (const char *filename, const struct grub_dirhook_info *info,
 }
 
 static grub_err_t
-grub_ls_list_files (char *dirname, int longlist, int all, int human)
+grub_ls_list_files (char *dirname, int longlist, int all, int human,
+                    char *varname)
 {
   char *device_name;
   grub_fs_t fs;
   const char *path;
   grub_device_t dev;
 
+  if (varname)
+    {
+      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "set and list files");
+      return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    }
+
   device_name = grub_file_get_device_name (dirname);
   dev = grub_device_open (device_name);
   if (! dev)
@@ -278,11 +323,13 @@ grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
   int i;
 
   if (argc == 0)
-    grub_ls_list_devices (state[0].set);
+    grub_ls_list_devices (state[1].set,
+			  state[0].set ? state[0].arg : NULL);
   else
     for (i = 0; i < argc; i++)
-      grub_ls_list_files (args[i], state[0].set, state[2].set,
-			  state[1].set);
+      grub_ls_list_files (args[i], state[1].set, state[3].set,
+			  state[2].set,
+			  state[0].set ? state[0].arg : NULL);
 
   return 0;
 }
-- 
2.45.1


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

[RFC][PATCH v1 4/4] commands/ls: support --set for files/directories.

[Denis 'GNUtoo' Carikli]

The ls command has no way to get the name of the files or directories
being listed inside an environment variable.

This enables to programmatically, inside the grub.cfg be able to look
for files and react if they are found or not found.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/commands/ls.c | 192 ++++++++++++++++++++++++++++++++++------
 1 file changed, 166 insertions(+), 26 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 113235781..8a6528caa 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -127,6 +127,7 @@ struct grub_ls_list_files_ctx
   char *dirname;
   int all;
   int human;
+  char *varname;
 };
 
 /* Helper for grub_ls_list_files.  */
@@ -137,7 +138,17 @@ print_files (const char *filename, const struct grub_dirhook_info *info,
   struct grub_ls_list_files_ctx *ctx = data;
 
   if (ctx->all || filename[0] != '.')
-    grub_printf ("%s%s ", filename, info->dir ? "/" : "");
+  {
+    if (ctx->varname)
+      {
+	grub_env_append (ctx->varname, filename);
+	grub_env_append (ctx->varname, info->dir ? "/" : "");
+      }
+    else
+      {
+	grub_printf ("%s%s ", filename, info->dir ? "/" : "");
+      }
+  }
 
   return 0;
 }
@@ -158,9 +169,26 @@ print_files_long (const char *filename, const struct grub_dirhook_info *info,
       char *pathname;
 
       if (ctx->dirname[grub_strlen (ctx->dirname) - 1] == '/')
-	pathname = grub_xasprintf ("%s%s", ctx->dirname, filename);
+	if (ctx->varname)
+	  {
+	    grub_env_append (ctx->varname, ctx->dirname);
+	    grub_env_append (ctx->varname, filename);
+	  }
+	else
+	  {
+	    pathname = grub_xasprintf ("%s%s", ctx->dirname, filename);
+	  }
       else
-	pathname = grub_xasprintf ("%s/%s", ctx->dirname, filename);
+	if (ctx->varname)
+	  {
+	    grub_env_append (ctx->varname, ctx->dirname);
+	    grub_env_append (ctx->varname, "/");
+	    grub_env_append (ctx->varname, filename);
+	  }
+	else
+	  {
+	    pathname = grub_xasprintf ("%s/%s", ctx->dirname, filename);
+	  }
 
       if (!pathname)
 	return 1;
@@ -172,38 +200,144 @@ print_files_long (const char *filename, const struct grub_dirhook_info *info,
       if (file)
 	{
 	  if (! ctx->human)
-	    grub_printf ("%-12llu", (unsigned long long) file->size);
+	    if (ctx->varname)
+	      {
+		char *str = grub_xasprintf("%-12llu",
+					   (unsigned long long) file->size);
+		if (str)
+		  {
+		    grub_env_append (ctx->varname, str);
+		    grub_free (str);
+		  }
+		else
+		  {
+		    grub_error (GRUB_ERR_OUT_OF_MEMORY,
+				"print_files_long :grub_xasprintf failed.");
+		    return 0;
+		  }
+	      }
+	    else
+	      {
+		grub_printf ("%-12llu", (unsigned long long) file->size);
+	      }
 	  else
-	    grub_printf ("%-12s", grub_get_human_size (file->size,
-						   GRUB_HUMAN_SIZE_SHORT));
+	    if (ctx->varname)
+	      {
+		char *str = grub_xasprintf("%-12s",
+					   grub_get_human_size (file->size,
+								GRUB_HUMAN_SIZE_SHORT));
+		if (str)
+		  {
+		    grub_env_append (ctx->varname, str);
+		    grub_free (str);
+		  }
+		else
+		  {
+		    grub_error (GRUB_ERR_OUT_OF_MEMORY,
+				"print_files_long: grub_xasprintf failed.");
+		    return 0;
+		  }
+	      }
+	    else
+	      {
+		grub_printf ("%-12s", grub_get_human_size (file->size,
+							   GRUB_HUMAN_SIZE_SHORT));
+	      }
 	  grub_file_close (file);
 	}
       else
-	grub_xputs ("????????????");
+	if (ctx->varname)
+	  grub_env_append (ctx->varname, "????????????");
+	else
+	  grub_xputs ("????????????");
 
       grub_free (pathname);
       grub_errno = GRUB_ERR_NONE;
     }
   else
-    grub_printf ("%-12s", _("DIR"));
+    if (ctx->varname)
+      {
+	char *str = grub_xasprintf("%-12s", _("DIR"));
+	if (str)
+	  {
+	    grub_env_append (ctx->varname, str);
+	    grub_free (str);
+	  }
+	else
+	  {
+	    grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			"print_files_long: grub_xasprintf failed.");
+	    return 0;
+	  }
+      }
+    else
+      {
+	grub_printf ("%-12s", _("DIR"));
+      }
 
   if (info->mtimeset)
     {
       struct grub_datetime datetime;
       grub_unixtime2datetime (info->mtime, &datetime);
       if (ctx->human)
-	grub_printf (" %d-%02d-%02d %02d:%02d:%02d %-11s ",
-		     datetime.year, datetime.month, datetime.day,
-		     datetime.hour, datetime.minute,
-		     datetime.second,
-		     grub_get_weekday_name (&datetime));
+	if (ctx->varname) {
+	  char *str = grub_xasprintf(" %d-%02d-%02d %02d:%02d:%02d %-11s ",
+				     datetime.year, datetime.month,
+				     datetime.day,
+				     datetime.hour, datetime.minute,
+				     datetime.second,
+				     grub_get_weekday_name (&datetime));
+	  if (str) {
+	    grub_env_append (ctx->varname, str);
+	    grub_free (str);
+	  } else {
+	    grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			"print_files_long: grub_xasprintf failed.");
+	    return 0;
+	  }
+	} else {
+	  grub_printf (" %d-%02d-%02d %02d:%02d:%02d %-11s ",
+		       datetime.year, datetime.month, datetime.day,
+		       datetime.hour, datetime.minute,
+		       datetime.second,
+		       grub_get_weekday_name (&datetime));
+	}
       else
-	grub_printf (" %04d%02d%02d%02d%02d%02d ",
-		     datetime.year, datetime.month,
-		     datetime.day, datetime.hour,
-		     datetime.minute, datetime.second);
+	if (ctx->varname)
+	  {
+	    char *str = grub_xasprintf(" %04d%02d%02d%02d%02d%02d ",
+				       datetime.year, datetime.month,
+				       datetime.day, datetime.hour,
+				       datetime.minute, datetime.second);
+	    if (str)
+	      {
+		grub_env_append (ctx->varname, str);
+		grub_free (str);
+	      }
+	    else
+	      {
+		grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			    "print_files_long: grub_xasprintf failed.");
+		return 0;
+	      }
+	  }
+	else
+	  {
+	    grub_printf (" %04d%02d%02d%02d%02d%02d ",
+			 datetime.year, datetime.month,
+			 datetime.day, datetime.hour,
+			 datetime.minute, datetime.second);
+	  }
+    }
+  if (ctx->varname)
+    {
+      grub_env_append (ctx->varname, filename);
+      grub_env_append (ctx->varname, info->dir ? "/" : "");
+    }
+  else
+    {
+      grub_printf ("%s%s\n", filename, info->dir ? "/" : "");
     }
-  grub_printf ("%s%s\n", filename, info->dir ? "/" : "");
 
   return 0;
 }
@@ -217,12 +351,6 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human,
   const char *path;
   grub_device_t dev;
 
-  if (varname)
-    {
-      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "set and list files");
-      return GRUB_ERR_NOT_IMPLEMENTED_YET;
-    }
-
   device_name = grub_file_get_device_name (dirname);
   dev = grub_device_open (device_name);
   if (! dev)
@@ -243,6 +371,11 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human,
 
   if (! *path && device_name)
     {
+      if (varname) {
+	grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "set and device infos");
+        goto fail;
+      }
+
       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
 	grub_errno = GRUB_ERR_NONE;
 
@@ -262,9 +395,13 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human,
       struct grub_ls_list_files_ctx ctx = {
 	.dirname = dirname,
 	.all = all,
-	.human = human
+	.human = human,
+	.varname = varname
       };
 
+      if (varname)
+	      grub_env_unset(varname);
+
       if (longlist)
 	(fs->fs_dir) (dev, path, print_files_long, &ctx);
       else
@@ -302,7 +439,10 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human,
 	}
 
       if (grub_errno == GRUB_ERR_NONE)
-	grub_xputs ("\n");
+	if (varname)
+	  grub_env_append (varname, "\n");
+	else
+	  grub_xputs ("\n");
 
       grub_refresh ();
     }
-- 
2.45.1


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

Re: [RFC][PATCH v1 0/4] How to add --set=VARNAME to the ls command?

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 4395 bytes --]

Did you try:
insmod regexp
for x in (*); do
....
done
Just trying to understand the problem

Le dim. 30 juin 2024, 19:26, Denis 'GNUtoo' Carikli <
GNUtoo@cyberdimension.org> a écrit :

> Hi,
>
> The problem we try to solve with --set=VARNAME in ls.
> =====================================================
> In the GNU Boot project (a free software distribution that releases
> free software boot firmware images), we provide images with (a
> deblobbed) Coreboot and GRUB (run as a Coreboot payload). We use GRUB
> mainly to find other configuration files like syslinux.cfg (to boot on
> external medias) or grub.cfg (to boot on the (usually GNU/Linux)
> distribution installed to the hard disk / SSD).
>
> We also provide images with a SeaBIOS Coreboot payload instead, but we
> plan to make the images with GRUB become the preffered way of booting
> because in practice it works very well with the Coreboot Framebuffer,
> and with it we only lack a way to reliabily list the devices being
> present in order to be able to also find grub.cfg config files inside
> filesystems present on LVM logical volumes as well.
>
> The alternative to using GRUB as a Coreboot payload is to use SeaBIOS
> instead but that doesn't work well because when SeaBIOS loads the
> (usually GNU/Linux) distribution's GRUB, it results in a black screen
> unless the users tweak the /etc/default/grub configuration to use the
> 'console' output instead of the default gfxterm, and we also want less
> technical users to be able to easily use computers with GNU Boot. This
> issue is probably due to SeaVGABIOS that probably doesn't fully
> implement the VGA standard, so my guess is that fixing this is more
> work than adding --set=VARNAME to the 'ls' command.
>
> Our current GRUB configuration file is in our git repository[1] and it
> hardcodes devices like ahciX,Y and then tries to find the grub.cfg
> with (a limited) number of X,Y combination.
>
> [1]
> https://git.savannah.gnu.org/cgit/gnuboot.git/tree/resources/grub/config/grub.cfg
>
> Questions about the implementation
> ==================================
> The patch set that follows is far from optimal:
>
> * The 'commands/ls: add --set=VARNAME.' patch only implements
>   --set=VARNAME for 'ls' without other arguments, and it returns an
>   error otherwise. I'm not sure if it's the right solution but in
>   another hand implementing --set=VARNAME for all the ls command would
>   make the patch too big given how the implementation is done (more
>   on that later).
>
> * The patches adding --set=VARNAME 'commands/ls: add --set=VARNAME.'
>   changes is not very intrusive but the later patch 'commands/ls:
>   support --set for files/directories.' shows the broader issue very
>   clearly: all the prints are duplicated with some 'if (varname) {
>   ... }' construct.
>
> Since here my goal is only to add '--set=VARNAME' for 'ls' without
> arguments, what would be the best way to proceed?
>
> Would a patch that doesn't cover all the 'ls' arguments be acceptable?
> If not, I guess that the way to go would be to rework a bit the
> printing as with the current way, there is too much duplication of
> code and it also makes the code harder to follow which in turn makes
> maintenance of this code harder.
>
> In this case what kind of API would be acceptable? Should we introduce
> some functions that have an argument that can select where to print?
>
> If so would something similar to fprintf be ok? It could be used like
> that 'grub_xfprintf( varname ? stdout : varname, "%s\n", "Hello
> world");' and make the code more redable than with the 'commands/ls:
> support --set for files/directories.' patch.
>
> Denis 'GNUtoo' Carikli (4):
>   Add grub_env_append function.
>   Add command to append to existing environment variables.
>   commands/ls: add --set=VARNAME.
>   commands/ls: support --set for files/directories.
>
>  grub-core/commands/ls.c  | 249 ++++++++++++++++++++++++++++++++++-----
>  grub-core/kern/corecmd.c |  25 ++++
>  grub-core/kern/env.c     |  38 ++++++
>  include/grub/env.h       |   1 +
>  4 files changed, 282 insertions(+), 31 deletions(-)
>
> --
> 2.45.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 5451 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [RFC][PATCH v1 0/4] How to add --set=VARNAME to the ls command?

[Denis 'GNUtoo' Carikli]

[-- Attachment #1.1: Type: text/plain, Size: 363 bytes --]

On Sun, 30 Jun 2024 20:05:10 +0300
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> Did you try:
> insmod regexp
> for x in (*); do
> ....
> done
> Just trying to understand the problem
I didn't know that was possible, the command you gave me works fine and
fits the bill even better than my half-baked patches.

Thanks a lot.

Denis.

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

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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