Le ven. 6 oct. 2023, 10:25, Glenn Washburn <developm...@efficientek.com> a écrit :
> 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. > 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 > > > > >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
