Re: [PATCH v3 2/2] fat: Support file modification times
On Mon, Mar 9, 2020 at 7:33 AM Daniel Kiper wrote:
> On Sat, Mar 07, 2020 at 12:59:52AM -0500, David Michael wrote:
> > This allows comparing file ages on EFI system partitions.
> >
> > Signed-off-by: David Michael
>
> Reviewed-by: Daniel Kiper
>
> ...except...
>
> > ---
> >
> > Changes since v2:
> > * Added comments referencing the specs
> > * Set errno when the timestamp is invalid
> >
> > I set errno rather than print to the console since it looks like most
> > other file systems don't tend to write to the console. I also went with
> > GRUB_ERR_OUT_OF_RANGE since that error only occurs when a time field is
> > above the valid range, but maybe GRUB_ERR_BAD_FS belongs there.
> >
> > grub-core/fs/fat.c | 56 ++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> > index dc493add2..24a47e2df 100644
> > --- a/grub-core/fs/fat.c
> > +++ b/grub-core/fs/fat.c
> > @@ -26,6 +26,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #ifndef MODE_EXFAT
> > #include
> > #else
> > @@ -730,6 +731,31 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
> >return grub_errno ? : GRUB_ERR_EOF;
> > }
> >
> > +/* Convert a timestamp in exFAT format to seconds since the UNIX epoch
> > + according to sections 7.4.8 and 7.4.9 in the exFAT specification.
> > +
> > https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification */
>
> The comment is formated incorrectly...
>
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
>
> > +static int
> > +grub_exfat_timestamp (grub_uint32_t field, grub_uint8_t msec, grub_int32_t
> > *nix) {
> > + struct grub_datetime datetime = {
> > +.year = (field >> 25) + 1980,
> > +.month = (field & 0x01E0) >> 21,
> > +.day= (field & 0x001F) >> 16,
> > +.hour = (field & 0xF800) >> 11,
> > +.minute = (field & 0x07E0) >> 5,
> > +.second = (field & 0x001F) * 2 + (msec >= 100 ? 1 : 0),
> > + };
> > +
> > + /* The conversion below allows seconds=60, so don't trust its
> > validation. */
> > + if ((field & 0x1F) > 29)
> > +return 0;
> > +
> > + /* Validate the 10-msec field even though it is rounded down to seconds.
> > */
> > + if (msec > 199)
> > +return 0;
> > +
> > + return grub_datetime2unixtime (, nix);
> > +}
> > +
> > #else
> >
> > static grub_err_t
> > @@ -857,6 +883,27 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
> >return grub_errno ? : GRUB_ERR_EOF;
> > }
> >
> > +/* Convert a date and time in FAT format to seconds since the UNIX epoch
> > + according to sections 11.3.5 and 11.3.6 in ECMA-107.
> > +
> > https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-107.pdf
> > */
>
> Ditto...
>
> This time I can fix them both before commiting if you wish...
Yes, please. I just wrote them that way to be consistent with the
other multiline comments in that file.
Thanks.
David
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging
On Mon, Mar 09, 2020 at 01:22:34PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 09, 2020 at 12:18:41PM +0100, Paul Menzel wrote: > > Dear Patrick, > > > > On 2020-03-07 17:29, Patrick Steinhardt wrote: > > > The memory management system supports a debug mode that can be enabled > > > at build time by passing "--enable-mm-debug" to the configure script. > > > Passing the option will cause us define MM_DEBUG as expected, but in > > > fact the reverse option "--disable-mm-deubg" will do the exact same > > > > s/deubg/debug/ > > Thanks for pointing this out! Daniel, you want me to send out a v2 of > this to fix the typo or will you fix it up locally? I will fix it locally... Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging
On Mon, Mar 09, 2020 at 12:18:41PM +0100, Paul Menzel wrote: > Dear Patrick, > > > On 2020-03-07 17:29, Patrick Steinhardt wrote: > > The memory management system supports a debug mode that can be enabled > > at build time by passing "--enable-mm-debug" to the configure script. > > Passing the option will cause us define MM_DEBUG as expected, but in > > fact the reverse option "--disable-mm-deubg" will do the exact same > > s/deubg/debug/ Thanks for pointing this out! Daniel, you want me to send out a v2 of this to fix the typo or will you fix it up locally? Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging
On Mon, Mar 09, 2020 at 01:01:51PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 09, 2020 at 12:19:15PM +0100, Daniel Kiper wrote: > > On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote: > > > When building GRUB with memory management debugging enabled, then the > > > build fails because of `grub_debug_malloc()` and `grub_debug_free()` > > > being undefined in the luks2 module. The cause is that we patch > > > "base64.h" to unconditionaly include "config-util.h", which shouldn't be > > > included for modules at all. As a result, `MM_DEBUG` is defined when > > > building the module, causing it to use the debug memory allocation > > > functions. As these are not built into modules, we end up with a linker > > > error. > > > > > > Fix the issue by removing the include altogether. The > > > sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro, > > > which we can simply define as empty in case it's not set. > > > > > > Signed-off-by: Patrick Steinhardt > > > --- > > > grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++ > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch > > > b/grub-core/lib/gnulib-patches/fix-base64.patch > > > index e075b6fab..985db1279 100644 > > > --- a/grub-core/lib/gnulib-patches/fix-base64.patch > > > +++ b/grub-core/lib/gnulib-patches/fix-base64.patch > > > @@ -1,14 +1,8 @@ > > > diff --git a/lib/base64.h b/lib/base64.h > > > -index 9cd0183b8..a2aaa2d4a 100644 > > > +index 9cd0183b8..185a2afa1 100644 > > > --- a/lib/base64.h > > > +++ b/lib/base64.h > > > -@@ -18,11 +18,16 @@ > > > - #ifndef BASE64_H > > > - # define BASE64_H > > > > Hmmm... It seems to me that you should not drop this... > > Note that this is a diff of a patch. So all that's getting dropped is > the patch that added the #include. So after applying this, "base64.h" > doesn't get touched by fix-base64.patch at all anymore. Ahhh... Right, these are reference lines. So, sorry for the confusion... Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging
On Mon, Mar 09, 2020 at 12:19:15PM +0100, Daniel Kiper wrote: > On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote: > > When building GRUB with memory management debugging enabled, then the > > build fails because of `grub_debug_malloc()` and `grub_debug_free()` > > being undefined in the luks2 module. The cause is that we patch > > "base64.h" to unconditionaly include "config-util.h", which shouldn't be > > included for modules at all. As a result, `MM_DEBUG` is defined when > > building the module, causing it to use the debug memory allocation > > functions. As these are not built into modules, we end up with a linker > > error. > > > > Fix the issue by removing the include altogether. The > > sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro, > > which we can simply define as empty in case it's not set. > > > > Signed-off-by: Patrick Steinhardt > > --- > > grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++ > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch > > b/grub-core/lib/gnulib-patches/fix-base64.patch > > index e075b6fab..985db1279 100644 > > --- a/grub-core/lib/gnulib-patches/fix-base64.patch > > +++ b/grub-core/lib/gnulib-patches/fix-base64.patch > > @@ -1,14 +1,8 @@ > > diff --git a/lib/base64.h b/lib/base64.h > > -index 9cd0183b8..a2aaa2d4a 100644 > > +index 9cd0183b8..185a2afa1 100644 > > --- a/lib/base64.h > > +++ b/lib/base64.h > > -@@ -18,11 +18,16 @@ > > - #ifndef BASE64_H > > - # define BASE64_H > > Hmmm... It seems to me that you should not drop this... Note that this is a diff of a patch. So all that's getting dropped is the patch that added the #include. So after applying this, "base64.h" doesn't get touched by fix-base64.patch at all anymore. Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 2/2] fat: Support file modification times
On Sat, Mar 07, 2020 at 12:59:52AM -0500, David Michael wrote:
> This allows comparing file ages on EFI system partitions.
>
> Signed-off-by: David Michael
Reviewed-by: Daniel Kiper
...except...
> ---
>
> Changes since v2:
> * Added comments referencing the specs
> * Set errno when the timestamp is invalid
>
> I set errno rather than print to the console since it looks like most
> other file systems don't tend to write to the console. I also went with
> GRUB_ERR_OUT_OF_RANGE since that error only occurs when a time field is
> above the valid range, but maybe GRUB_ERR_BAD_FS belongs there.
>
> grub-core/fs/fat.c | 56 ++
> 1 file changed, 56 insertions(+)
>
> diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> index dc493add2..24a47e2df 100644
> --- a/grub-core/fs/fat.c
> +++ b/grub-core/fs/fat.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #ifndef MODE_EXFAT
> #include
> #else
> @@ -730,6 +731,31 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>return grub_errno ? : GRUB_ERR_EOF;
> }
>
> +/* Convert a timestamp in exFAT format to seconds since the UNIX epoch
> + according to sections 7.4.8 and 7.4.9 in the exFAT specification.
> + https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
> */
The comment is formated incorrectly...
https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
> +static int
> +grub_exfat_timestamp (grub_uint32_t field, grub_uint8_t msec, grub_int32_t
> *nix) {
> + struct grub_datetime datetime = {
> +.year = (field >> 25) + 1980,
> +.month = (field & 0x01E0) >> 21,
> +.day= (field & 0x001F) >> 16,
> +.hour = (field & 0xF800) >> 11,
> +.minute = (field & 0x07E0) >> 5,
> +.second = (field & 0x001F) * 2 + (msec >= 100 ? 1 : 0),
> + };
> +
> + /* The conversion below allows seconds=60, so don't trust its validation.
> */
> + if ((field & 0x1F) > 29)
> +return 0;
> +
> + /* Validate the 10-msec field even though it is rounded down to seconds.
> */
> + if (msec > 199)
> +return 0;
> +
> + return grub_datetime2unixtime (, nix);
> +}
> +
> #else
>
> static grub_err_t
> @@ -857,6 +883,27 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>return grub_errno ? : GRUB_ERR_EOF;
> }
>
> +/* Convert a date and time in FAT format to seconds since the UNIX epoch
> + according to sections 11.3.5 and 11.3.6 in ECMA-107.
> +
> https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-107.pdf */
Ditto...
This time I can fix them both before commiting if you wish...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 1/2] exfat: Save the matching directory entry struct when searching
On Sat, Mar 07, 2020 at 12:59:31AM -0500, David Michael wrote: > This provides the node's attributes outside the iterator function > so the file modification time can be accessed and reported. > > Signed-off-by: David Michael Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 08/12] kern: Make grub_error() more verbose
On Fri, Mar 06, 2020 at 02:37:43PM +0100, Vladimir 'phcoder' Serbinenko wrote: > Le ven. 6 mars 2020 à 13:43, Daniel Kiper a écrit : > > > Re-adding grub-devel@gnu.org... > > > > On Fri, Mar 06, 2020 at 12:44:05PM +0100, Vladimir 'phcoder' Serbinenko > > wrote: > > > Le ven. 6 mars 2020 à 12:42, Daniel Kiper a écrit > > : > > > > > > > On Thu, Mar 05, 2020 at 03:22:31PM +0100, Vladimir 'phcoder' Serbinenko > > > > wrote: > > > > > Please evaluate size increase for this. In the past passing file and > > line > > > > > number to grub_dprintf was a huge source of increased Kern and core > > size > > > > > > > > I think it does not matter much if we stop pretending that we support > > > > small MBR gaps right now [1]. Does it? > > > > > > > There are still a lot of installations that used older tools and never > > > reformatted. We still care about them > > > > If we go that way then we have to care about them by the end of the > > universe. And this means more and more issues like [1]. If somebody > > wants to use new GRUB then he/she have to reinstall the machine or > > something like that. IMO we should not care about users who do not want > > upgrade their machines or whatnot. Or at least their choices cannot > > impact GRUB development too much. And I think that this MBR constraint > > is hindering the project too much at this point. So, as above... > > > > Sorry for being blunt... > > > It does not. Core doesn't need to have everything and the kitchen sink. > It's small by design. I am not saying that. I am saying that limitations of one ancient platform should not make development of other platforms more difficult. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging
Dear Patrick, On 2020-03-07 17:29, Patrick Steinhardt wrote: > The memory management system supports a debug mode that can be enabled > at build time by passing "--enable-mm-debug" to the configure script. > Passing the option will cause us define MM_DEBUG as expected, but in > fact the reverse option "--disable-mm-deubg" will do the exact same s/deubg/debug/ > thing and also set up the define. This currently causes the build of > "lib/gnulib/base64.c" to fail as it tries to use `grub_debug_malloc()` > and `grub_debug_free()` even though both symbols aren't defined. > > Seemingly, `AC_ARG_ENABLE()` will always execute the third argument if > either the positive or negative option was passed. Let's thus fix the > issue by moving the call to`AC_DEFINE()` into an explicit `if test > $xenable_mm_debug` block, similar to how other defines work. > > Signed-off-by: Patrick Steinhardt > --- > configure.ac | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/configure.ac b/configure.ac > index f19489418..9eeec2d76 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1448,9 +1448,11 @@ LIBS="$tmp_LIBS" > # Memory manager debugging. > AC_ARG_ENABLE([mm-debug], > AS_HELP_STRING([--enable-mm-debug], > - [include memory manager debugging]), > - [AC_DEFINE([MM_DEBUG], [1], > - [Define to 1 if you enable memory manager > debugging.])]) > + [include memory manager debugging])) > +if test x$enable_mm_debug = xyes; then > +AC_DEFINE([MM_DEBUG], [1], > +[Define to 1 if you enable memory manager debugging.]) > +fi > > AC_ARG_ENABLE([cache-stats], > AS_HELP_STRING([--enable-cache-stats], Reviewed-by: Paul Menzel Kind regards, Paul smime.p7s Description: S/MIME Cryptographic Signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging
On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote:
> When building GRUB with memory management debugging enabled, then the
> build fails because of `grub_debug_malloc()` and `grub_debug_free()`
> being undefined in the luks2 module. The cause is that we patch
> "base64.h" to unconditionaly include "config-util.h", which shouldn't be
> included for modules at all. As a result, `MM_DEBUG` is defined when
> building the module, causing it to use the debug memory allocation
> functions. As these are not built into modules, we end up with a linker
> error.
>
> Fix the issue by removing the include altogether. The
> sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro,
> which we can simply define as empty in case it's not set.
>
> Signed-off-by: Patrick Steinhardt
> ---
> grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch
> b/grub-core/lib/gnulib-patches/fix-base64.patch
> index e075b6fab..985db1279 100644
> --- a/grub-core/lib/gnulib-patches/fix-base64.patch
> +++ b/grub-core/lib/gnulib-patches/fix-base64.patch
> @@ -1,14 +1,8 @@
> diff --git a/lib/base64.h b/lib/base64.h
> -index 9cd0183b8..a2aaa2d4a 100644
> +index 9cd0183b8..185a2afa1 100644
> --- a/lib/base64.h
> +++ b/lib/base64.h
> -@@ -18,11 +18,16 @@
> - #ifndef BASE64_H
> - # define BASE64_H
Hmmm... It seems to me that you should not drop this...
> -
> -+/* Get _GL_ATTRIBUTE_CONST */
> -+# include
> -+
> +@@ -21,8 +21,14 @@
> /* Get size_t. */
> # include
>
> @@ -17,6 +11,10 @@ index 9cd0183b8..a2aaa2d4a 100644
> +#ifndef GRUB_POSIX_BOOL_DEFINED
> +typedef enum { false = 0, true = 1 } bool;
> +#define GRUB_POSIX_BOOL_DEFINED 1
> ++#endif
> ++
> ++#ifndef _GL_ATTRIBUTE_CONST
> ++# define _GL_ATTRIBUTE_CONST /* empty */
> +#endif
>
> # ifdef __cplusplus
> --
> 2.25.1
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging
On Sat, Mar 07, 2020 at 05:29:08PM +0100, Patrick Steinhardt wrote: > The memory management system supports a debug mode that can be enabled > at build time by passing "--enable-mm-debug" to the configure script. > Passing the option will cause us define MM_DEBUG as expected, but in > fact the reverse option "--disable-mm-deubg" will do the exact same > thing and also set up the define. This currently causes the build of > "lib/gnulib/base64.c" to fail as it tries to use `grub_debug_malloc()` > and `grub_debug_free()` even though both symbols aren't defined. > > Seemingly, `AC_ARG_ENABLE()` will always execute the third argument if > either the positive or negative option was passed. Let's thus fix the > issue by moving the call to`AC_DEFINE()` into an explicit `if test > $xenable_mm_debug` block, similar to how other defines work. > > Signed-off-by: Patrick Steinhardt Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
