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
> >