On Thu, 14 Sep 2023 15:08:00 -0500 Glenn Washburn <developm...@efficientek.com> wrote:
> On Thu, 14 Sep 2023 17:37:10 +0200 > "Vladimir 'phcoder' Serbinenko" <phco...@gmail.com> wrote: > > > Le lun. 14 août 2023, 20:58, Glenn Washburn <developm...@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
