[PATCH v3 2/2] fat: Support file modification times
This allows comparing file ages on EFI system partitions.
Signed-off-by: David Michael
---
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 */
+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
*/
+static int
+grub_fat_timestamp (grub_uint16_t time, grub_uint16_t date, grub_int32_t *nix)
{
+ struct grub_datetime datetime = {
+.year = (date >> 9) + 1980,
+.month = (date & 0x01E0) >> 5,
+.day= (date & 0x001F),
+.hour = (time >> 11),
+.minute = (time & 0x07E0) >> 5,
+.second = (time & 0x001F) * 2,
+ };
+
+ /* The conversion below allows seconds=60, so don't trust its validation. */
+ if ((time & 0x1F) > 29)
+return 0;
+
+ return grub_datetime2unixtime (, nix);
+}
+
#endif
static grub_err_t lookup_file (grub_fshelp_node_t node,
@@ -966,10 +1013,19 @@ grub_fat_dir (grub_device_t device, const char *path,
grub_fs_dir_hook_t hook,
#ifdef MODE_EXFAT
if (!ctxt.dir.have_stream)
continue;
+ info.mtimeset = grub_exfat_timestamp (grub_le_to_cpu32
(ctxt.entry.type_specific.file.m_time),
+
ctxt.entry.type_specific.file.m_time_tenth,
+ );
#else
if (ctxt.dir.attr & GRUB_FAT_ATTR_VOLUME_ID)
continue;
+ info.mtimeset = grub_fat_timestamp (grub_le_to_cpu16 (ctxt.dir.w_time),
+ grub_le_to_cpu16 (ctxt.dir.w_date),
+ );
#endif
+ if (info.mtimeset == 0)
+ grub_error (GRUB_ERR_OUT_OF_RANGE,
+ "invalid modification timestamp for %s", path);
if (hook (ctxt.filename, , hook_data))
break;
--
2.21.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 1/2] exfat: Save the matching directory entry struct when searching
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
---
Changes since v2:
* Updated commit message
grub-core/fs/fat.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
index d544e0af1..dc493add2 100644
--- a/grub-core/fs/fat.c
+++ b/grub-core/fs/fat.c
@@ -596,6 +596,7 @@ struct grub_fat_iterate_context
{
#ifdef MODE_EXFAT
struct grub_fat_dir_node dir;
+ struct grub_fat_dir_entry entry;
#else
struct grub_fat_dir_entry dir;
#endif
@@ -642,27 +643,27 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
grub_memset (>dir, 0, sizeof (ctxt->dir));
while (1)
{
- struct grub_fat_dir_entry dir;
+ struct grub_fat_dir_entry *dir = >entry;
- ctxt->offset += sizeof (dir);
+ ctxt->offset += sizeof (*dir);
- if (grub_fat_read_data (node->disk, node, 0, 0, ctxt->offset, sizeof
(dir),
- (char *) )
- != sizeof (dir))
+ if (grub_fat_read_data (node->disk, node, 0, 0, ctxt->offset, sizeof
(*dir),
+ (char *) dir)
+ != sizeof (*dir))
break;
- if (dir.entry_type == 0)
+ if (dir->entry_type == 0)
break;
- if (!(dir.entry_type & 0x80))
+ if (!(dir->entry_type & 0x80))
continue;
- if (dir.entry_type == 0x85)
+ if (dir->entry_type == 0x85)
{
unsigned i, nsec, slots = 0;
- nsec = dir.type_specific.file.secondary_count;
+ nsec = dir->type_specific.file.secondary_count;
- ctxt->dir.attr = grub_cpu_to_le16 (dir.type_specific.file.attr);
+ ctxt->dir.attr = grub_cpu_to_le16 (dir->type_specific.file.attr);
ctxt->dir.have_stream = 0;
for (i = 0; i < nsec; i++)
{
@@ -705,7 +706,7 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
if (i != nsec)
{
- ctxt->offset -= sizeof (dir);
+ ctxt->offset -= sizeof (*dir);
continue;
}
@@ -715,16 +716,16 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
return 0;
}
/* Allocation bitmap. */
- if (dir.entry_type == 0x81)
+ if (dir->entry_type == 0x81)
continue;
/* Upcase table. */
- if (dir.entry_type == 0x82)
+ if (dir->entry_type == 0x82)
continue;
/* Volume label. */
- if (dir.entry_type == 0x83)
+ if (dir->entry_type == 0x83)
continue;
grub_dprintf ("exfat", "unknown primary type 0x%02x\n",
- dir.entry_type);
+ dir->entry_type);
}
return grub_errno ? : GRUB_ERR_EOF;
}
--
2.21.1
___
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
> It would be best to use a boot partition so the core.img space is > reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an > existing UEFI GPT disk, so I wrote the commands that do what you're > describing here: > > https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178 > Just use gdisk to add a partition into this 1MiB hole. you only need to ask it to reduce alignment. ___ 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
Le 06/03/2020 à 18:03, David Michael a écrit : > On Fri, Mar 6, 2020 at 9:02 AM Didier Spaier wrote: >> Le 06/03/2020 à 13:43, Daniel Kiper a écrit : >>> 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... >> >> Sorry to be off topic... In case of a GUID partition table, if I >> understand the UEFI specification[1], as the first usable LBA should be >> greater than or equal to 34 for a 512 bytes block size or 6 for a >> 4096 bytes logical block size, it could begin after a gap of 24K. >> >> Then, if we assume that the first partition begins @ 1MiB, can't GRUB >> use the space unused between the first usable LBA and 1MiB instead of >> a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as >> then a Bios Boot partition wouldn't be necessary any more. > > It would be best to use a boot partition so the core.img space is > reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an > existing UEFI GPT disk, so I wrote the commands that do what you're > describing here: > > https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178 > > I may be misremembering, but I think the core.img size was limited to > around half a megabyte, so it should be safe to write it to the unused > ~1MiB before the first partition after the GPT. But yes, using the > boot partition would probably be for the best if you are formatting a > new disk. Thanks for sharing David. This can be useful to allow Legacy booting off a drive initially partitioned for UEFI booting only. ___ 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 06:10:58PM +0100, Didier Spaier wrote: > My mistake, I should have written starting at 24 KiB as that is the > start address of the first usable LBA if I read correctly the UEFI > specification. Am I wrong? > > Thanks for the heads-up. I was wrong about 32KB, it is 32 sectors of 512 when that is the block size plus the MBR and header sectors, so 34 sectors at 512b, and yes with 4K blocks you hit 24k. Not sure if bigger sector sizes are likely to happen at some point. Maybe 32 or 64KB would be a safer starting point and being a power of 2, slightly more likely to hit a disk aligned block size. -- Len Sorensen ___ 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 11:12:14AM -0600, Bruce Dubbs wrote: > Just a terminology issue here. Please don't call it a boot partition. It > conflicts with those who mount /boot as a separate partition. Please call > it a grub partition. Well the official name is "BIOS boot partition", so I can see why people tend to call it a boot partition. -- Len Sorensen ___ 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 3/6/20 11:03 AM, David Michael wrote: On Fri, Mar 6, 2020 at 9:02 AM Didier Spaier wrote: Le 06/03/2020 à 13:43, Daniel Kiper a écrit : 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... Sorry to be off topic... In case of a GUID partition table, if I understand the UEFI specification[1], as the first usable LBA should be greater than or equal to 34 for a 512 bytes block size or 6 for a 4096 bytes logical block size, it could begin after a gap of 24K. Then, if we assume that the first partition begins @ 1MiB, can't GRUB use the space unused between the first usable LBA and 1MiB instead of a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as then a Bios Boot partition wouldn't be necessary any more. It would be best to use a boot partition so the core.img space is reserved by the GPT, Just a terminology issue here. Please don't call it a boot partition. It conflicts with those who mount /boot as a separate partition. Please call it a grub partition. -- Bruce but I once wanted to tack i386-pc GRUB onto an existing UEFI GPT disk, so I wrote the commands that do what you're describing here: https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178 I may be misremembering, but I think the core.img size was limited to around half a megabyte, so it should be safe to write it to the unused ~1MiB before the first partition after the GPT. But yes, using the boot partition would probably be for the best if you are formatting a new disk. Thanks. David ___ 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
Re: [PATCH v2 08/12] kern: Make grub_error() more verbose
Le 06/03/2020 à 17:39, Lennart Sorensen a écrit : > On Fri, Mar 06, 2020 at 05:30:23PM +0100, Didier Spaier wrote: >> That'd be just one less thing to care about for the Slint installer >> in case of an 'automatic' installation, nothing major. >> >> Do you confirm that a Bios Boot partition starting at 1 KiB >> and ending at 1 MiB won't overlap the GPT and is big enough? > > It would absolutely overlap GPT since GPT usually starts at 512 bytes > and goes up to 32KB or so. My mistake, I should have written starting at 24 KiB as that is the start address of the first usable LBA if I read correctly the UEFI specification. Am I wrong? Thanks for the heads-up. ___ 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 6, 2020 at 9:02 AM Didier Spaier wrote: > Le 06/03/2020 à 13:43, Daniel Kiper a écrit : > > 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... > > Sorry to be off topic... In case of a GUID partition table, if I > understand the UEFI specification[1], as the first usable LBA should be > greater than or equal to 34 for a 512 bytes block size or 6 for a > 4096 bytes logical block size, it could begin after a gap of 24K. > > Then, if we assume that the first partition begins @ 1MiB, can't GRUB > use the space unused between the first usable LBA and 1MiB instead of > a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as > then a Bios Boot partition wouldn't be necessary any more. It would be best to use a boot partition so the core.img space is reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an existing UEFI GPT disk, so I wrote the commands that do what you're describing here: https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178 I may be misremembering, but I think the core.img size was limited to around half a megabyte, so it should be safe to write it to the unused ~1MiB before the first partition after the GPT. But yes, using the boot partition would probably be for the best if you are formatting a new disk. Thanks. David ___ 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 05:30:23PM +0100, Didier Spaier wrote: > That'd be just one less thing to care about for the Slint installer > in case of an 'automatic' installation, nothing major. > > Do you confirm that a Bios Boot partition starting at 1 KiB > and ending at 1 MiB won't overlap the GPT and is big enough? It would absolutely overlap GPT since GPT usually starts at 512 bytes and goes up to 32KB or so. -- Len Sorensen ___ 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
Le 06/03/2020 à 15:38, Vladimir 'phcoder' Serbinenko a écrit : > What's the problem with having bios boot partition? You can put it > exactly in the spot you describe (in fact I do so) and it works as > well, just additionally it marks this space as occupied That'd be just one less thing to care about for the Slint installer in case of an 'automatic' installation, nothing major. Do you confirm that a Bios Boot partition starting at 1 KiB and ending at 1 MiB won't overlap the GPT and is big enough? Thanks, Didier ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 2/2] fat: Support file modification times
On Fri, Mar 06, 2020 at 10:48:07AM -0500, David Michael wrote:
> On Fri, Mar 6, 2020 at 7:19 AM Daniel Kiper wrote:
> > On Tue, Mar 03, 2020 at 02:41:08PM -0500, David Michael wrote:
> > > This allows comparing file ages on EFI system partitions.
> > >
> > > Signed-off-by: David Michael
> > > ---
> > >
> > > Changes since v1:
> > > - Added the previous patch to help support exfat
> > > - Added exfat timestamp conversion + setting
> > > - Switched to datetime variable name for consistency with the header
> > > - Switched to tabs-for-alignment for consistency in the file
> > >
> > > grub-core/fs/fat.c | 47 ++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> > > index dc493add2..bacf9e60f 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,28 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
> > >return grub_errno ? : GRUB_ERR_EOF;
> > > }
> > >
> > > +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. */
> >
> > 60 seconds is a valid value in case of leap second. Hence, the question
> > is: Can 60 seconds be represented properly in exFAT somehow? OK, this
> > does not happen often. So, if we want ignore that case then at least
> > I would like to have an explanation that we ignore it due to...
>
> I enforced the 0-59 range because that is what is declared valid in
> the spec. See 11.3.5 in ECMA-107[1] and 7.4.8 for exfat[2].
OK, could you add references to these documents into the comments?
> > > + if ((field & 0x1F) > 29)
> > > +return 0;
> >
> > You silently ignore this error. Should not you spit something to the
> > console in this case? Or maybe at least set grub_errno? This way
> > user will know that result of comparison should not be trusted...
>
> The functions also rely on the grub_datetime2unixtime field
> validations, which do not print errors. I can add a general
> grub_error if info.mtimeset is zero so it warns of all failures.
Works for me.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 2/2] fat: Support file modification times
On Fri, Mar 6, 2020 at 7:19 AM Daniel Kiper wrote:
> On Tue, Mar 03, 2020 at 02:41:08PM -0500, David Michael wrote:
> > This allows comparing file ages on EFI system partitions.
> >
> > Signed-off-by: David Michael
> > ---
> >
> > Changes since v1:
> > - Added the previous patch to help support exfat
> > - Added exfat timestamp conversion + setting
> > - Switched to datetime variable name for consistency with the header
> > - Switched to tabs-for-alignment for consistency in the file
> >
> > grub-core/fs/fat.c | 47 ++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> > index dc493add2..bacf9e60f 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,28 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
> >return grub_errno ? : GRUB_ERR_EOF;
> > }
> >
> > +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. */
>
> 60 seconds is a valid value in case of leap second. Hence, the question
> is: Can 60 seconds be represented properly in exFAT somehow? OK, this
> does not happen often. So, if we want ignore that case then at least
> I would like to have an explanation that we ignore it due to...
I enforced the 0-59 range because that is what is declared valid in
the spec. See 11.3.5 in ECMA-107[1] and 7.4.8 for exfat[2].
> > + if ((field & 0x1F) > 29)
> > +return 0;
>
> You silently ignore this error. Should not you spit something to the
> console in this case? Or maybe at least set grub_errno? This way
> user will know that result of comparison should not be trusted...
The functions also rely on the grub_datetime2unixtime field
validations, which do not print errors. I can add a general
grub_error if info.mtimeset is zero so it warns of all failures.
Thanks.
David
[1] https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-107.pdf
[2] https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
___
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 6, 2020 at 3:02 PM Didier Spaier wrote: > > Le 06/03/2020 à 13:43, Daniel Kiper a écrit : > > 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... > > Sorry to be off topic... In case of a GUID partition table, if I > understand the UEFI specification[1], as the first usable LBA should be > greater than or equal to 34 for a 512 bytes block size or 6 for a > 4096 bytes logical block size, it could begin after a gap of 24K. > > Then, if we assume that the first partition begins @ 1MiB, can't GRUB > use the space unused between the first usable LBA and 1MiB instead of > a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as > then a Bios Boot partition wouldn't be necessary any more. What's the problem with having bios boot partition? You can put it exactly in the spot you describe (in fact I do so) and it works as well, just additionally it marks this space as occupied > > I just hope this question is not too silly... > > Best regards, > Didier > > > [1]https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf > § 5.3.1 GPT overview > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel -- Regards Vladimir 'phcoder' Serbinenko ___ 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
Le 06/03/2020 à 13:43, Daniel Kiper a écrit : > 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... Sorry to be off topic... In case of a GUID partition table, if I understand the UEFI specification[1], as the first usable LBA should be greater than or equal to 34 for a 512 bytes block size or 6 for a 4096 bytes logical block size, it could begin after a gap of 24K. Then, if we assume that the first partition begins @ 1MiB, can't GRUB use the space unused between the first usable LBA and 1MiB instead of a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as then a Bios Boot partition wouldn't be necessary any more. I just hope this question is not too silly... Best regards, Didier [1]https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf § 5.3.1 GPT overview ___ 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
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. > > Daniel > > [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html > ___ 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
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... Daniel [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 2/2] fat: Support file modification times
On Tue, Mar 03, 2020 at 02:41:08PM -0500, David Michael wrote:
> This allows comparing file ages on EFI system partitions.
>
> Signed-off-by: David Michael
> ---
>
> Changes since v1:
> - Added the previous patch to help support exfat
> - Added exfat timestamp conversion + setting
> - Switched to datetime variable name for consistency with the header
> - Switched to tabs-for-alignment for consistency in the file
>
> grub-core/fs/fat.c | 47 ++
> 1 file changed, 47 insertions(+)
>
> diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> index dc493add2..bacf9e60f 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,28 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>return grub_errno ? : GRUB_ERR_EOF;
> }
>
> +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.
> */
60 seconds is a valid value in case of leap second. Hence, the question
is: Can 60 seconds be represented properly in exFAT somehow? OK, this
does not happen often. So, if we want ignore that case then at least
I would like to have an explanation that we ignore it due to...
> + if ((field & 0x1F) > 29)
> +return 0;
You silently ignore this error. Should not you spit something to the
console in this case? Or maybe at least set grub_errno? This way
user will know that result of comparison should not be trusted...
> + /* Validate the 10-msec field even though it is rounded down to seconds.
> */
> + if (msec > 199)
> +return 0;
Ditto...
> + return grub_datetime2unixtime (, nix);
> +}
> +
> #else
>
> static grub_err_t
> @@ -857,6 +880,24 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>return grub_errno ? : GRUB_ERR_EOF;
> }
>
> +static int
> +grub_fat_timestamp (grub_uint16_t time, grub_uint16_t date, grub_int32_t
> *nix) {
> + struct grub_datetime datetime = {
> +.year = (date >> 9) + 1980,
> +.month = (date & 0x01E0) >> 5,
> +.day= (date & 0x001F),
> +.hour = (time >> 11),
> +.minute = (time & 0x07E0) >> 5,
> +.second = (time & 0x001F) * 2,
> + };
> +
> + /* The conversion below allows seconds=60, so don't trust its validation.
> */
> + if ((time & 0x1F) > 29)
> +return 0;
Ditto...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/2] exfat: Save the matching directory entry when iterating
On Tue, Mar 03, 2020 at 02:40:54PM -0500, David Michael wrote: > This provides the node's attributes outside the search function. > > Signed-off-by: David Michael Reviewed-by: Daniel Kiper ...except one nit... > --- > > I had to copy an extra struct to support exfat mtimes, so I put it in a > separate patch for readability. Next time please add explanation why, like above one, to the proper commit message. This time I will move it for you. 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 3/6/20 12:41 PM, Vladimir 'phcoder' Serbinenko wrote:
> Le ven. 6 mars 2020 à 12:38, Javier Martinez Canillas
> a écrit :
>
>> Hello Vladimir,
>>
>> Thanks a lot for your feedback.
>>
>> On 3/5/20 3:22 PM, 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
>>>
>>> Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <
>> javi...@redhat.com>
>>> a écrit :
>>>
>>
>> For a x86_64-pc build with platform=pc on master with and without the
>> patch:
>>
>> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
>> 2740total
>>
>> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
>> 2756total
>>
>> so in this case the increase is a 0.6% in size.
>>
> This is not the comparison we care about. We care about kernel.img size and
> core.img size for several common configs. And in bytes.
> We have only 31K in mbr gap and every byte counts
>
Fair. Then I'll only do the comparison for pc platform since for EFI it
doesn't matter due the binary being in the ESP.
I built a core.img with the ext2, part_msdos and biosdisk modules which I
think is a pretty common configuration used. So the following command:
$ ./grub-mkimage --directory ./grub-core/ --prefix '(,msdos1)/grub2' --output
core.img --format 'i386-pc' --compression 'auto' 'ext2' 'part_msdos' 'biosdisk'
and I got the following sizes in bytes for the two cases:
without the patch: 25977
with the patch: 26350
So I think my comment still applies, it's just a 1.4% increase (373 bytes).
But also as Daniel said, in most cases the gap between the end of the MBR
and the start of the first partition will be at least 1 MiB due partitions
being aligned to a 1 MiB boundary. It shouldn't be an issue for that case.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
___
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
Le ven. 6 mars 2020 à 12:38, Javier Martinez Canillas
a écrit :
> Hello Vladimir,
>
> Thanks a lot for your feedback.
>
> On 3/5/20 3:22 PM, 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
> >
> > Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <
> javi...@redhat.com>
> > a écrit :
> >
>
> For a x86_64-pc build with platform=pc on master with and without the
> patch:
>
> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
> 2740total
>
> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
> 2756total
>
> so in this case the increase is a 0.6% in size.
>
This is not the comparison we care about. We care about kernel.img size and
core.img size for several common configs. And in bytes.
We have only 31K in mbr gap and every byte counts
___
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 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? Daniel [1] https://lists.gnu.org/archive/html/grub-devel/2020-03/msg00059.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] docs: Update for stopping small mbr gap support
On Fri, Mar 06, 2020 at 11:38:49AM +0100, Javier Martinez Canillas wrote:
> Hello Michael,
>
> On 3/6/20 7:53 AM, Michael Chang wrote:
> > Further to the discussion about disabling btrfs zstd support for
> > i386-pc[1], this paragraph in manual about mbr gap size doesn't seem to
> > hold true any longer.
> >
> > "You must ensure that the first partition starts at least 31 KiB (63
> > sectors) from the start of the disk"
> >
> > As in many occasions we inevitablely have to provide core image with the
> > size that goes beyond 31 KiB. For instance, diskfilter and crypto
> > modules which are needed by root disk formatted with btrfs, lvm, mdadm
> > and so on would add quite a lot space to the image.
> >
> > So this misinformation would have people misguided and thought that it
> > is still fine to use small MBR gap utill some point of time the update
> > has grown the size too much that the grub-install can no longer embed
> > the image to the mbr gap. In this case changing the partition layout is
> > required but it is never easy to do so.
> >
> > The patch tries to correct the paragraph with a more practical size that
> > works for grub and also for modern computer systems in general.
> >
> > [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html
> >
> > Signed-off-by: Michael Chang
> > ---
> >
>
> Agreed with the patch.
>
> Reviewed-by: Javier Martinez Canillas
Reviewed-by: Daniel Kiper
...except some nitpicks which I fix before commiting the patch...
> > Changes since v2:
> > * Rework a paragraph in commit message and also some places in manual
> >to be more clear to read
> > * Correct some typos
> >
> > docs/grub.texi | 20 ++--
> > 1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 83979af38..4614a2ee1 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -845,12 +845,20 @@ only be used if the @file{/boot} filesystem is on the
> > same disk that the
> > BIOS boots from, so that GRUB does not have to rely on guessing BIOS drive
> > numbers.
> >
> > -The GRUB development team generally recommends embedding GRUB before the
> > -first partition, unless you have special requirements. You must ensure
> > that
> > -the first partition starts at least 31 KiB (63 sectors) from the start of
> > -the disk; on modern disks, it is often a performance advantage to align
> > -partitions on larger boundaries anyway, so the first partition might start
> > 1
> > -MiB from the start of the disk.
> > +The GRUB development team generally recommends embedding GRUB before the
> > first
> > +partition, unless you have special requirements. You must ensure that the
> > first
> > +partition starts at least 1 MiB from the start of the disk; Additionally,
> > on
> > +modern disks it is often a performance advantage to align partitions on
> > larger
> > +boundaries and 1 MiB is the least common multiple of many used alignment
> > sizes.
> > +E.g. SSD, it became crucial to have the partition correctly aligned to
> > avoid
> > +excessive read-modify-write cycles and thus modern tools set to use 1 MiB
> > as a
> > +standard practice.
> > +
> > +In case of legacy systems that cannot boot if first partition is not on the
> > +cylinder boundary, the fallback blocklist install method should remain
> > working
> > +for them if the core image grows too much someday. Here we just can't
> > advertise
> > +that 31 KiB (63 sectors) is a sensible size any longer as that would pose
> > great
> > +constraint to include new features as time goes by.
>
> I think is also worth mentioning that most partition tools these days default
> to
> the optimal alignment for the disk. But could be done as a follow-up patch if
> you
> agree that adding this information would be useful.
This is not a must. However, if you think it makes sense please send a patch.
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
Hello Vladimir,
Thanks a lot for your feedback.
On 3/5/20 3:22 PM, 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
>
> Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas
> a écrit :
>
For a x86_64-pc build with platform=pc on master with and without the patch:
$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
2740total
$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
2756total
so in this case the increase is a 0.6% in size.
And doing the same in a x86_64 with platform=efi build:
$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
4296total
$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
4356total
The size increase it's a little bigger but just a 1.4%.
You are right that there's a size increase but I wouldn't call it huge and
I think that's a good trade off for the convenience of having that info.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Enable the datetime module for the emu platform
On Fri, Mar 06, 2020 at 11:09:08AM +0100, Javier Martinez Canillas wrote: > Hello Mike, > > On 3/5/20 10:52 PM, Mike Gilbert wrote: > > Fixes a build failure: > > > > grub-core/commands/date.c:49: undefined reference to `grub_get_weekday_name' > > grub-core/commands/ls.c:155: undefined reference to `grub_unixtime2datetime' > > > > Bug: https://bugs.gentoo.org/711512 > > Signed-off-by: Mike Gilbert > > --- > > Patch looks good to me and it fixes the issue. > > Reviewed-by: Javier Martinez Canillas > Tested-by: Javier Martinez Canillas Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] docs: Update for stopping small mbr gap support
Hello Michael,
On 3/6/20 7:53 AM, Michael Chang wrote:
> Further to the discussion about disabling btrfs zstd support for
> i386-pc[1], this paragraph in manual about mbr gap size doesn't seem to
> hold true any longer.
>
> "You must ensure that the first partition starts at least 31 KiB (63
> sectors) from the start of the disk"
>
> As in many occasions we inevitablely have to provide core image with the
> size that goes beyond 31 KiB. For instance, diskfilter and crypto
> modules which are needed by root disk formatted with btrfs, lvm, mdadm
> and so on would add quite a lot space to the image.
>
> So this misinformation would have people misguided and thought that it
> is still fine to use small MBR gap utill some point of time the update
> has grown the size too much that the grub-install can no longer embed
> the image to the mbr gap. In this case changing the partition layout is
> required but it is never easy to do so.
>
> The patch tries to correct the paragraph with a more practical size that
> works for grub and also for modern computer systems in general.
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html
>
> Signed-off-by: Michael Chang
> ---
>
Agreed with the patch.
Reviewed-by: Javier Martinez Canillas
> Changes since v2:
> * Rework a paragraph in commit message and also some places in manual
>to be more clear to read
> * Correct some typos
>
> docs/grub.texi | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 83979af38..4614a2ee1 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -845,12 +845,20 @@ only be used if the @file{/boot} filesystem is on the
> same disk that the
> BIOS boots from, so that GRUB does not have to rely on guessing BIOS drive
> numbers.
>
> -The GRUB development team generally recommends embedding GRUB before the
> -first partition, unless you have special requirements. You must ensure that
> -the first partition starts at least 31 KiB (63 sectors) from the start of
> -the disk; on modern disks, it is often a performance advantage to align
> -partitions on larger boundaries anyway, so the first partition might start 1
> -MiB from the start of the disk.
> +The GRUB development team generally recommends embedding GRUB before the
> first
> +partition, unless you have special requirements. You must ensure that the
> first
> +partition starts at least 1 MiB from the start of the disk; Additionally, on
> +modern disks it is often a performance advantage to align partitions on
> larger
> +boundaries and 1 MiB is the least common multiple of many used alignment
> sizes.
> +E.g. SSD, it became crucial to have the partition correctly aligned to avoid
> +excessive read-modify-write cycles and thus modern tools set to use 1 MiB as
> a
> +standard practice.
> +
> +In case of legacy systems that cannot boot if first partition is not on the
> +cylinder boundary, the fallback blocklist install method should remain
> working
> +for them if the core image grows too much someday. Here we just can't
> advertise
> +that 31 KiB (63 sectors) is a sensible size any longer as that would pose
> great
> +constraint to include new features as time goes by.
>
I think is also worth mentioning that most partition tools these days default to
the optimal alignment for the disk. But could be done as a follow-up patch if
you
agree that adding this information would be useful.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Enable the datetime module for the emu platform
Hello Mike, On 3/5/20 10:52 PM, Mike Gilbert wrote: > Fixes a build failure: > > grub-core/commands/date.c:49: undefined reference to `grub_get_weekday_name' > grub-core/commands/ls.c:155: undefined reference to `grub_unixtime2datetime' > > Bug: https://bugs.gentoo.org/711512 > Signed-off-by: Mike Gilbert > --- Patch looks good to me and it fixes the issue. Reviewed-by: Javier Martinez Canillas Tested-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
