Re: [PATCH 0/4] More ls improvements

From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
To: Glenn Washburn <development@efficientek.com>
Cc: The development of GNU GRUB <grub-devel@gnu.org>,
	Daniel Kiper <dkiper@net-space.pl>
Subject: Re: [PATCH 0/4] More ls improvements
Date: Fri, 1 Mar 2024 18:36:20 +0300	[thread overview]
Message-ID: <CAEaD8JPeaq-2iPJ9Fc45dZFdK+LYSrjkbQry0XxigK091_9d7g@mail.gmail.com> (raw)
In-Reply-To: <20231006022544.10f897c6@crass-HP-ZBook-15-G2>

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