[PATCH 0/4] More ls improvements

[PATCH 0/4] More ls improvements

[Glenn Washburn]

Currently when given a path to a file, ls will open the file to determine
if its is valid and then run the appropriate print function, in contrast to
directory arguments that use the directory iterator and callback on each
file. One issue with this is that opening a file does not allow access to
its modification time information, whereas the info object from the callback
called by the directory iterator does and the longlist print function will
print the modification time if present. The result is that when longlisting
ls arguments, directory arguments show moditication times but file arguments
do not. Patch 2 rectifies this an in the process simplifies the code path
by using the directory iterator for file arguments as well.

The implementation of patch 2 exposed a bug in grub_disk_read() which is
fixed in patch 1.

Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
ls output. And patch 4 also fixes an issue where there are blank lines
between consecutive file arguments.

Glenn

Glenn Washburn (4):
  disk: Reset grub_errno upon entering grub_disk_read()
  commands/ls: Allow printing mtime for file arguments
  commands/ls: Add directory header for dir args and print full paths
    for file args
  commands/ls: Proper line breaks between arguments

 grub-core/commands/ls.c | 117 +++++++++++++++++++++++-----------------
 grub-core/kern/disk.c   |   2 +
 2 files changed, 71 insertions(+), 48 deletions(-)

-- 
2.34.1


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

[PATCH 1/4] disk: Reset grub_errno upon entering grub_disk_read()

[Glenn Washburn]

The grub_disk_read() function returns grub_errno. Without resetting
grub_errno when entering the function, grub_disk_read() might return
a previously set error code when it should return success.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/disk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 1eda58fe9a1c..438909e21d07 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -417,6 +417,8 @@ grub_err_t
 grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		grub_off_t offset, grub_size_t size, void *buf)
 {
+  grub_errno = GRUB_ERR_NONE;
+
   /* First of all, check if the region is within the disk.  */
   if (grub_disk_adjust_range (disk, &sector, &offset, size) != GRUB_ERR_NONE)
     {
-- 
2.34.1


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

[PATCH 2/4] commands/ls: Allow printing mtime for file arguments

[Glenn Washburn]

File arguments were processed differently than files listed from directory
arguments. A side effect of this was that mtime was not shown for file
arguments when long listing was enabled. Refactor to have the same code
path for printing files that are arguments and ones that are contained in
directories. This also has the added benefit of simplifying the code path.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 70 ++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 6a1c7f5d3626..bc84c66b3060 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -87,8 +87,10 @@ grub_ls_list_devices (int longlist)
 struct grub_ls_list_files_ctx
 {
   char *dirname;
+  char *filename;
   int all;
   int human;
+  int longlist;
 };
 
 /* Helper for grub_ls_list_files.  */
@@ -98,22 +100,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 ? "/" : "");
-
-  return 0;
-}
-
-/* Helper for grub_ls_list_files.  */
-static int
-print_files_long (const char *filename, const struct grub_dirhook_info *info,
-		  void *data)
-{
-  struct grub_ls_list_files_ctx *ctx = data;
-
   if ((! ctx->all) && (filename[0] == '.'))
     return 0;
 
+  if (! ctx->longlist)
+    {
+      if (ctx->filename != NULL)
+	grub_xputs (ctx->dirname);
+      grub_printf ("%s%s ", filename, info->dir ? "/" : "");
+      return 0;
+    }
+
   if (! info->dir)
     {
       grub_file_t file;
@@ -170,6 +167,19 @@ print_files_long (const char *filename, const struct grub_dirhook_info *info,
   return 0;
 }
 
+/* Helper for grub_ls_list_files.  */
+static int
+print_file (const char *filename, const struct grub_dirhook_info *info,
+	    void *data)
+{
+  struct grub_ls_list_files_ctx *ctx = data;
+
+  if (grub_strcmp (filename, ctx->filename) != 0)
+    return 0;
+
+  return print_files (filename, info, data);
+}
+
 static grub_err_t
 grub_ls_list_files (char *dirname, int longlist, int all, int human)
 {
@@ -216,42 +226,24 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
     {
       struct grub_ls_list_files_ctx ctx = {
 	.dirname = dirname,
+	.filename = NULL,
 	.all = all,
-	.human = human
+	.human = human,
+	.longlist = longlist
       };
 
-      if (longlist)
-	(fs->fs_dir) (dev, path, print_files_long, &ctx);
-      else
-	(fs->fs_dir) (dev, path, print_files, &ctx);
+      (fs->fs_dir) (dev, path, print_files, &ctx);
 
       if (grub_errno == GRUB_ERR_BAD_FILE_TYPE
 	  && path[grub_strlen (path) - 1] != '/')
 	{
 	  /* PATH might be a regular file.  */
-	  char *p;
-	  grub_file_t file;
-	  struct grub_dirhook_info info;
-	  grub_errno = 0;
-
-	  file = grub_file_open (dirname, GRUB_FILE_TYPE_GET_SIZE
-				 | GRUB_FILE_TYPE_NO_DECOMPRESS);
-	  if (! file)
-	    goto fail;
-
-	  grub_file_close (file);
-
-	  p = grub_strrchr (dirname, '/') + 1;
-	  ctx.dirname = grub_strndup (dirname, p - dirname);
+	  ctx.filename = grub_strrchr (dirname, '/') + 1;
+	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
 	  if (ctx.dirname == NULL)
 	    goto fail;
 
-	  all = 1;
-	  grub_memset (&info, 0, sizeof (info));
-	  if (longlist)
-	    print_files_long (p, &info, &ctx);
-	  else
-	    print_files (p, &info, &ctx);
+	  (fs->fs_dir) (dev, ctx.dirname, print_file, &ctx);
 
 	  grub_free (ctx.dirname);
 	}
@@ -268,7 +260,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 
   grub_free (device_name);
 
-  return 0;
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
-- 
2.34.1


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

[PATCH 3/4] commands/ls: Add directory header for dir args and print full paths for file args

[Glenn Washburn]

Like the GNU ls first print a line with the directory path before printing
files in the directory, which will not have a directory component, but only
if there is more than one argument. Also, for arguments that are paths to
files, print the full path of the file.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index bc84c66b3060..fc1ee3281841 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -91,6 +91,7 @@ struct grub_ls_list_files_ctx
   int all;
   int human;
   int longlist;
+  int print_dirhdr;
 };
 
 /* Helper for grub_ls_list_files.  */
@@ -98,11 +99,18 @@ static int
 print_files (const char *filename, const struct grub_dirhook_info *info,
 	     void *data)
 {
+  char *pathname = NULL;
   struct grub_ls_list_files_ctx *ctx = data;
 
   if ((! ctx->all) && (filename[0] == '.'))
     return 0;
 
+  if (ctx->print_dirhdr)
+    {
+      grub_printf ("%s:\n", ctx->dirname);
+      ctx->print_dirhdr = 0;
+    }
+
   if (! ctx->longlist)
     {
       if (ctx->filename != NULL)
@@ -114,7 +122,6 @@ print_files (const char *filename, const struct grub_dirhook_info *info,
   if (! info->dir)
     {
       grub_file_t file;
-      char *pathname;
 
       if (ctx->dirname[grub_strlen (ctx->dirname) - 1] == '/')
 	pathname = grub_xasprintf ("%s%s", ctx->dirname, filename);
@@ -140,7 +147,6 @@ print_files (const char *filename, const struct grub_dirhook_info *info,
       else
 	grub_xputs ("????????????");
 
-      grub_free (pathname);
       grub_errno = GRUB_ERR_NONE;
     }
   else
@@ -162,7 +168,10 @@ print_files (const char *filename, const struct grub_dirhook_info *info,
 		     datetime.day, datetime.hour,
 		     datetime.minute, datetime.second);
     }
-  grub_printf ("%s%s\n", filename, info->dir ? "/" : "");
+  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
+			 info->dir ? "/" : "");
+
+  grub_free (pathname);
 
   return 0;
 }
@@ -181,7 +190,7 @@ print_file (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, int dirhdr)
 {
   char *device_name;
   grub_fs_t fs;
@@ -229,7 +238,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	.filename = NULL,
 	.all = all,
 	.human = human,
-	.longlist = longlist
+	.longlist = longlist,
+	.print_dirhdr = dirhdr
       };
 
       (fs->fs_dir) (dev, path, print_files, &ctx);
@@ -238,6 +248,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	  && path[grub_strlen (path) - 1] != '/')
 	{
 	  /* PATH might be a regular file.  */
+	  ctx.print_dirhdr = 0;
 	  ctx.filename = grub_strrchr (dirname, '/') + 1;
 	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
 	  if (ctx.dirname == NULL)
@@ -273,8 +284,8 @@ grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
     grub_ls_list_devices (state[0].set);
   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[0].set, state[2].set, state[1].set,
+			  argc > 1);
 
   return 0;
 }
-- 
2.34.1


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

[PATCH 4/4] commands/ls: Proper line breaks between arguments

[Glenn Washburn]

There should be a blank line before each directory argument and before
each file arugment that come after a directory argument. This brings the
ls command more inline with GNU's ls. Although one key difference still is
that GNU's ls reorders the output of arguments so that all file arguments
are grouped at the beginning of output, while GRUB's ls does no reordering
and shows output for each arument in the order that it was specified on
the command line. Also fixed is an issue where there was a blank line
between the output of consecutive file path arguments, which needlessly
wastes valuable screen space.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index fc1ee3281841..90fb4d2b9ec6 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -92,6 +92,8 @@ struct grub_ls_list_files_ctx
   int human;
   int longlist;
   int print_dirhdr;
+  int first_arg;
+  int prev_file;
 };
 
 /* Helper for grub_ls_list_files.  */
@@ -105,6 +107,19 @@ print_files (const char *filename, const struct grub_dirhook_info *info,
   if ((! ctx->all) && (filename[0] == '.'))
     return 0;
 
+  /* Never print a leading newline if this is the first arg */
+  if (! ctx->first_arg)
+    /*
+     * Print leading newline(s) if (this is a file argument and the previous
+     * was not) or this is the first file from a directory argument.
+     */
+    if (((ctx->filename != NULL) && (! ctx->prev_file)) || ctx->print_dirhdr)
+    {
+      grub_xputs ("\n");
+      if (! ctx->longlist)
+	grub_xputs ("\n");
+    }
+
   if (ctx->print_dirhdr)
     {
       grub_printf ("%s:\n", ctx->dirname);
@@ -190,7 +205,8 @@ print_file (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, int dirhdr)
+grub_ls_list_files (char *dirname, int longlist, int all, int human,
+		    int dirhdr, int first_arg, int *prev_file)
 {
   char *device_name;
   grub_fs_t fs;
@@ -239,7 +255,9 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human, int dirhdr)
 	.all = all,
 	.human = human,
 	.longlist = longlist,
-	.print_dirhdr = dirhdr
+	.print_dirhdr = dirhdr,
+	.first_arg = first_arg,
+	.prev_file = *prev_file
       };
 
       (fs->fs_dir) (dev, path, print_files, &ctx);
@@ -256,11 +274,11 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human, int dirhdr)
 
 	  (fs->fs_dir) (dev, ctx.dirname, print_file, &ctx);
 
+	  *prev_file = 1;
 	  grub_free (ctx.dirname);
 	}
-
-      if (grub_errno == GRUB_ERR_NONE)
-	grub_xputs ("\n");
+      else
+	*prev_file = 0;
 
       grub_refresh ();
     }
@@ -278,14 +296,14 @@ static grub_err_t
 grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
 {
   struct grub_arg_list *state = ctxt->state;
-  int i;
+  int i, prev_file = 0;
 
   if (argc == 0)
     grub_ls_list_devices (state[0].set);
   else
     for (i = 0; i < argc; i++)
       grub_ls_list_files (args[i], state[0].set, state[2].set, state[1].set,
-			  argc > 1);
+			  argc > 1, i == 0, &prev_file);
 
   return 0;
 }
-- 
2.34.1


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

Re: [PATCH 0/4] More ls improvements

[Daniel Kiper]

On Mon, Aug 14, 2023 at 01:57:06PM -0500, Glenn Washburn wrote:
> Currently when given a path to a file, ls will open the file to determine
> if its is valid and then run the appropriate print function, in contrast to
> directory arguments that use the directory iterator and callback on each
> file. One issue with this is that opening a file does not allow access to
> its modification time information, whereas the info object from the callback
> called by the directory iterator does and the longlist print function will
> print the modification time if present. The result is that when longlisting
> ls arguments, directory arguments show moditication times but file arguments
> do not. Patch 2 rectifies this an in the process simplifies the code path
> by using the directory iterator for file arguments as well.
>
> The implementation of patch 2 exposed a bug in grub_disk_read() which is
> fixed in patch 1.
>
> Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> ls output. And patch 4 also fixes an issue where there are blank lines
> between consecutive file arguments.

This series is nice improvement and does not pose significant regression
risk for the release. At least I cannot see it... :-)

So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel

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

Re: [PATCH 0/4] More ls improvements

[Vladimir 'phcoder' Serbinenko]

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

Le lun. 14 août 2023, 20:58, Glenn Washburn <development@efficientek.com> a
écrit :

> Currently when given a path to a file, ls will open the file to determine
> if its is valid and then run the appropriate print function, in contrast to
> directory arguments that use the directory iterator and callback on each
> file. One issue with this is that opening a file does not allow access to
> its modification time information, whereas the info object from the
> callback
> called by the directory iterator does and the longlist print function will
> print the modification time if present. The result is that when longlisting
> ls arguments, directory arguments show moditication times but file
> arguments
> do not. Patch 2 rectifies this an in the process simplifies the code path
> by using the directory iterator for file arguments as well.
>
> The implementation of patch 2 exposed a bug in grub_disk_read() which is
> fixed in patch 1.
>
> Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> ls output. And patch 4 also fixes an issue where there are blank lines
> between consecutive file arguments.
>
> Glenn
>
> Glenn Washburn (4):
>   disk: Reset grub_errno upon entering grub_disk_read()
>
Where does the error come from? We generally prefer to have
grub_print_error() (better) or resetting grib_errno after the error is
produced rather than blanketly reset grub_errno at the beginning

>   commands/ls: Allow printing mtime for file arguments
>   commands/ls: Add directory header for dir args and print full paths
>     for file args
>   commands/ls: Proper line breaks between arguments
>
>  grub-core/commands/ls.c | 117 +++++++++++++++++++++++-----------------
>  grub-core/kern/disk.c   |   2 +
>  2 files changed, 71 insertions(+), 48 deletions(-)
>
> --
> 2.34.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2759 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: [PATCH 0/4] More ls improvements

[Glenn Washburn]

On Thu, 14 Sep 2023 17:37:10 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> Le lun. 14 août 2023, 20:58, Glenn Washburn <development@efficientek.com> a
> écrit :
> 
> > Currently when given a path to a file, ls will open the file to determine
> > if its is valid and then run the appropriate print function, in contrast to
> > directory arguments that use the directory iterator and callback on each
> > file. One issue with this is that opening a file does not allow access to
> > its modification time information, whereas the info object from the
> > callback
> > called by the directory iterator does and the longlist print function will
> > print the modification time if present. The result is that when longlisting
> > ls arguments, directory arguments show moditication times but file
> > arguments
> > do not. Patch 2 rectifies this an in the process simplifies the code path
> > by using the directory iterator for file arguments as well.
> >
> > The implementation of patch 2 exposed a bug in grub_disk_read() which is
> > fixed in patch 1.
> >
> > Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> > ls output. And patch 4 also fixes an issue where there are blank lines
> > between consecutive file arguments.
> >
> > Glenn
> >
> > Glenn Washburn (4):
> >   disk: Reset grub_errno upon entering grub_disk_read()
> >
> Where does the error come from? We generally prefer to have
> grub_print_error() (better) or resetting grib_errno after the error is
> produced rather than blanketly reset grub_errno at the beginning

Take a look at patch 2, you'll see that the changes add another
(fs->fs_dir)(...). This added line is in an "if" block that is only
entered when grub_errno is not GRUB_ERR_SUCCESS. So under normal and
expected conditions, grub_errno will be set (when there are
non-directory arguments). This error is more of a flag than a real
error that should be shown the user.

The preferred usage policy of grub_errno that you suggest is aligned
with "man errno". So it has that going for it. I don't particularly
like it because, for one, I've seen odd read failures in the past that
I suspect are due to read returning with an error, when it actually
succeed. So it can give false positives that doesn't make sense and are
hard to track down. I see a few options here:

1. Have read() return err, instead of grub_errno, but that has a couple
problems. First, it probably will then ignore error from the cache
code. And second, I've not checked for usages of read() where
grub_errno is checked instead of the return value for error, but those
might exist as well.

2. Set grub_errno before every call to read(). But then that's not
really different that doing that at the start of the function.

3. A compromise option could be to output to the debug log a message
saying that read was called with grub_errno set. But that can easily be
overflowed, so it might not be noticed. And even if it is seen, what you
really care about is the caller, but is there a good way to get that
without having a debugger already attached? When the backtrace
functionality works again (perhaps soon), that could be used, but only
on x86.

4. In this specific instance set grub_errno before calling fs->fs_dir,
but then we still have the potential for this issue to exist elsewhere.
I believe I saw this happening on ppc when running one of the tests (I
think the functional test). But couldn't get to the bottom of it and now
I can't reproduce it (ie it probably got hidden rather than fixed).

If the concern is breaking things that currently work, we could opt for
option (4) with an eye to using option (1) after the release, giving
more focused testing. FWIW, I ran the tests for nearly all supported
architectures (notably not MIP, Loongson, or IA64) and no tests fails
because of this change. Also, I've been using this on x86 and seen no
issues because of it.

Glenn

> >   commands/ls: Allow printing mtime for file arguments
> >   commands/ls: Add directory header for dir args and print full paths
> >     for file args
> >   commands/ls: Proper line breaks between arguments
> >
> >  grub-core/commands/ls.c | 117 +++++++++++++++++++++++-----------------
> >  grub-core/kern/disk.c   |   2 +
> >  2 files changed, 71 insertions(+), 48 deletions(-)
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

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

Re: [PATCH 0/4] More ls improvements

[Glenn Washburn]

On Thu, 14 Sep 2023 15:08:00 -0500
Glenn Washburn <development@efficientek.com> wrote:

> On Thu, 14 Sep 2023 17:37:10 +0200
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:
> 
> > Le lun. 14 août 2023, 20:58, Glenn Washburn <development@efficientek.com> a
> > écrit :
> > 
> > > Currently when given a path to a file, ls will open the file to determine
> > > if its is valid and then run the appropriate print function, in contrast to
> > > directory arguments that use the directory iterator and callback on each
> > > file. One issue with this is that opening a file does not allow access to
> > > its modification time information, whereas the info object from the
> > > callback
> > > called by the directory iterator does and the longlist print function will
> > > print the modification time if present. The result is that when longlisting
> > > ls arguments, directory arguments show moditication times but file
> > > arguments
> > > do not. Patch 2 rectifies this an in the process simplifies the code path
> > > by using the directory iterator for file arguments as well.
> > >
> > > The implementation of patch 2 exposed a bug in grub_disk_read() which is
> > > fixed in patch 1.
> > >
> > > Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> > > ls output. And patch 4 also fixes an issue where there are blank lines
> > > between consecutive file arguments.
> > >
> > > Glenn
> > >
> > > Glenn Washburn (4):
> > >   disk: Reset grub_errno upon entering grub_disk_read()
> > >
> > Where does the error come from? We generally prefer to have
> > grub_print_error() (better) or resetting grib_errno after the error is
> > produced rather than blanketly reset grub_errno at the beginning
> 
> Take a look at patch 2, you'll see that the changes add another
> (fs->fs_dir)(...). This added line is in an "if" block that is only
> entered when grub_errno is not GRUB_ERR_SUCCESS. So under normal and
> expected conditions, grub_errno will be set (when there are
> non-directory arguments). This error is more of a flag than a real
> error that should be shown the user.
> 
> The preferred usage policy of grub_errno that you suggest is aligned
> with "man errno". So it has that going for it. I don't particularly

Actually, I take that back. My reading of the man page leads me to
conclude that library calls can do what they wish with errno, so I
think clearing it as in this patch is perfectly reasonable. The man
page states, if the errno needs to be used across another library call,
then it must be saved. The same should be expected of grub calls. I'll
admit its a bit of an apples and oranges comparison, but this makes
sense to me.

> like it because, for one, I've seen odd read failures in the past that
> I suspect are due to read returning with an error, when it actually
> succeed. So it can give false positives that doesn't make sense and are
> hard to track down. I see a few options here:
> 
> 1. Have read() return err, instead of grub_errno, but that has a couple
> problems. First, it probably will then ignore error from the cache
> code. And second, I've not checked for usages of read() where
> grub_errno is checked instead of the return value for error, but those
> might exist as well.
> 
> 2. Set grub_errno before every call to read(). But then that's not
> really different that doing that at the start of the function.
> 
> 3. A compromise option could be to output to the debug log a message
> saying that read was called with grub_errno set. But that can easily be
> overflowed, so it might not be noticed. And even if it is seen, what you
> really care about is the caller, but is there a good way to get that
> without having a debugger already attached? When the backtrace
> functionality works again (perhaps soon), that could be used, but only
> on x86.
> 
> 4. In this specific instance set grub_errno before calling fs->fs_dir,
> but then we still have the potential for this issue to exist elsewhere.
> I believe I saw this happening on ppc when running one of the tests (I
> think the functional test). But couldn't get to the bottom of it and now
> I can't reproduce it (ie it probably got hidden rather than fixed).
> 
> If the concern is breaking things that currently work, we could opt for
> option (4) with an eye to using option (1) after the release, giving
> more focused testing. FWIW, I ran the tests for nearly all supported
> architectures (notably not MIP, Loongson, or IA64) and no tests fails
> because of this change. Also, I've been using this on x86 and seen no
> issues because of it.
> 
> Glenn
> 
> > >   commands/ls: Allow printing mtime for file arguments
> > >   commands/ls: Add directory header for dir args and print full paths
> > >     for file args
> > >   commands/ls: Proper line breaks between arguments
> > >
> > >  grub-core/commands/ls.c | 117 +++++++++++++++++++++++-----------------
> > >  grub-core/kern/disk.c   |   2 +
> > >  2 files changed, 71 insertions(+), 48 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > Grub-devel mailing list
> > > Grub-devel@gnu.org
> > > https://lists.gnu.org/mailman/listinfo/grub-devel
> > >

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

Re: [PATCH 0/4] More ls improvements

[Glenn Washburn]

Hi Daniel,

On Thu, 14 Sep 2023 16:44:46 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Aug 14, 2023 at 01:57:06PM -0500, Glenn Washburn wrote:
> > Currently when given a path to a file, ls will open the file to determine
> > if its is valid and then run the appropriate print function, in contrast to
> > directory arguments that use the directory iterator and callback on each
> > file. One issue with this is that opening a file does not allow access to
> > its modification time information, whereas the info object from the callback
> > called by the directory iterator does and the longlist print function will
> > print the modification time if present. The result is that when longlisting
> > ls arguments, directory arguments show moditication times but file arguments
> > do not. Patch 2 rectifies this an in the process simplifies the code path
> > by using the directory iterator for file arguments as well.
> >
> > The implementation of patch 2 exposed a bug in grub_disk_read() which is
> > fixed in patch 1.
> >
> > Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> > ls output. And patch 4 also fixes an issue where there are blank lines
> > between consecutive file arguments.
> 
> This series is nice improvement and does not pose significant regression
> risk for the release. At least I cannot see it... :-)
> 
> So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Now that the release is out, where are we with this?

Glenn

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

Re: [PATCH 0/4] More ls improvements

[Daniel Kiper]

On Thu, Feb 29, 2024 at 02:57:34PM -0600, Glenn Washburn wrote:
> Hi Daniel,
>
> On Thu, 14 Sep 2023 16:44:46 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Mon, Aug 14, 2023 at 01:57:06PM -0500, Glenn Washburn wrote:
> > > Currently when given a path to a file, ls will open the file to determine
> > > if its is valid and then run the appropriate print function, in contrast to
> > > directory arguments that use the directory iterator and callback on each
> > > file. One issue with this is that opening a file does not allow access to
> > > its modification time information, whereas the info object from the callback
> > > called by the directory iterator does and the longlist print function will
> > > print the modification time if present. The result is that when longlisting
> > > ls arguments, directory arguments show moditication times but file arguments
> > > do not. Patch 2 rectifies this an in the process simplifies the code path
> > > by using the directory iterator for file arguments as well.
> > >
> > > The implementation of patch 2 exposed a bug in grub_disk_read() which is
> > > fixed in patch 1.
> > >
> > > Patches 3 and 4 aim to make the output of GRUB's ls look more like GNU's
> > > ls output. And patch 4 also fixes an issue where there are blank lines
> > > between consecutive file arguments.
> >
> > This series is nice improvement and does not pose significant regression
> > risk for the release. At least I cannot see it... :-)
> >
> > So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...
>
> Now that the release is out, where are we with this?

I have to check with Vladimir he is OK with your comments. Sorry for delays...

Daniel

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

Re: [PATCH 0/4] More ls improvements

[Vladimir 'phcoder' Serbinenko]

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

Le ven. 6 oct. 2023, 10:25, Glenn Washburn <development@efficientek.com> a
écrit :

> On Thu, 14 Sep 2023 15:08:00 -0500
> Glenn Washburn <development@efficientek.com> wrote:
>
> > On Thu, 14 Sep 2023 17:37:10 +0200
> > "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:
> >
> > > Le lun. 14 août 2023, 20:58, Glenn Washburn <
> development@efficientek.com> a
> > > écrit :
> > >
> > > > Currently when given a path to a file, ls will open the file to
> determine
> > > > if its is valid and then run the appropriate print function, in
> contrast to
> > > > directory arguments that use the directory iterator and callback on
> each
> > > > file. One issue with this is that opening a file does not allow
> access to
> > > > its modification time information, whereas the info object from the
> > > > callback
> > > > called by the directory iterator does and the longlist print
> function will
> > > > print the modification time if present. The result is that when
> longlisting
> > > > ls arguments, directory arguments show moditication times but file
> > > > arguments
> > > > do not. Patch 2 rectifies this an in the process simplifies the code
> path
> > > > by using the directory iterator for file arguments as well.
> > > >
> > > > The implementation of patch 2 exposed a bug in grub_disk_read()
> which is
> > > > fixed in patch 1.
> > > >
> > > > Patches 3 and 4 aim to make the output of GRUB's ls look more like
> GNU's
> > > > ls output. And patch 4 also fixes an issue where there are blank
> lines
> > > > between consecutive file arguments.
> > > >
> > > > Glenn
> > > >
> > > > Glenn Washburn (4):
> > > >   disk: Reset grub_errno upon entering grub_disk_read()
> > > >
> > > Where does the error come from? We generally prefer to have
> > > grub_print_error() (better) or resetting grib_errno after the error is
> > > produced rather than blanketly reset grub_errno at the beginning
> >
> > Take a look at patch 2, you'll see that the changes add another
> > (fs->fs_dir)(...). This added line is in an "if" block that is only
> > entered when grub_errno is not GRUB_ERR_SUCCESS. So under normal and
> > expected conditions, grub_errno will be set (when there are
> > non-directory arguments). This error is more of a flag than a real
> > error that should be shown the user.
> >
> > The preferred usage policy of grub_errno that you suggest is aligned
> > with "man errno". So it has that going for it. I don't particularly
>
> Actually, I take that back. My reading of the man page leads me to
> conclude that library calls can do what they wish with errno, so I
> think clearing it as in this patch is perfectly reasonable. The man
> page states, if the errno needs to be used across another library call,
> then it must be saved. The same should be expected of grub calls. I'll
> admit its a bit of an apples and oranges comparison, but this makes
> sense to me.
>


While grub_errno is named after posix, it's not designed in the same way.
It's more in line with the design of exceptions. I agree that this
situation is unfortunate and ideally we need to change it centrally. Let me
start a separate thread with possible options for this.

>
> > like it because, for one, I've seen odd read failures in the past that
> > I suspect are due to read returning with an error, when it actually
> > succeed. So it can give false positives that doesn't make sense and are
> > hard to track down. I see a few options here:
> >
> > 1. Have read() return err, instead of grub_errno, but that has a couple
> > problems. First, it probably will then ignore error from the cache
> > code. And second, I've not checked for usages of read() where
> > grub_errno is checked instead of the return value for error, but those
> > might exist as well.
> >
> > 2. Set grub_errno before every call to read(). But then that's not
> > really different that doing that at the start of the function.
> >
> > 3. A compromise option could be to output to the debug log a message
> > saying that read was called with grub_errno set. But that can easily be
> > overflowed, so it might not be noticed. And even if it is seen, what you
> > really care about is the caller, but is there a good way to get that
> > without having a debugger already attached? When the backtrace
> > functionality works again (perhaps soon), that could be used, but only
> > on x86.
> >
> > 4. In this specific instance set grub_errno before calling fs->fs_dir,
> > but then we still have the potential for this issue to exist elsewhere.
> > I believe I saw this happening on ppc when running one of the tests (I
> > think the functional test). But couldn't get to the bottom of it and now
> > I can't reproduce it (ie it probably got hidden rather than fixed).
> >
> > If the concern is breaking things that currently work, we could opt for
> > option (4) with an eye to using option (1) after the release, giving
> > more focused testing. FWIW, I ran the tests for nearly all supported
> > architectures (notably not MIP, Loongson, or IA64) and no tests fails
> > because of this change. Also, I've been using this on x86 and seen no
> > issues because of it.
> >
> > Glenn
> >
> > > >   commands/ls: Allow printing mtime for file arguments
> > > >   commands/ls: Add directory header for dir args and print full paths
> > > >     for file args
> > > >   commands/ls: Proper line breaks between arguments
> > > >
> > > >  grub-core/commands/ls.c | 117
> +++++++++++++++++++++++-----------------
> > > >  grub-core/kern/disk.c   |   2 +
> > > >  2 files changed, 71 insertions(+), 48 deletions(-)
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > > _______________________________________________
> > > > Grub-devel mailing list
> > > > Grub-devel@gnu.org
> > > > https://lists.gnu.org/mailman/listinfo/grub-devel
> > > >
>

[-- Attachment #1.2: Type: text/html, Size: 7784 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