On Fri, Mar 31, 2023 at 12:57:05AM +0000, Glenn Washburn wrote: > On 3/30/23 18:30, Daniel Kiper wrote: > > On Tue, Mar 07, 2023 at 05:56:49PM +0100, Thomas Schmitt wrote: > > > Hi, > > > > > > SUSP 1.12 says: > > > > > > The "CE" System Use Entry indicates a Continuation Area that shall be > > > processed after the current System Use field or Continuation Area is > > > processed. > > > > > > But GRUB rather takes an encountered CE entry as reason to immediately > > > switch reading to the location that is given by the CE entry. > > > This can skip over important information. > > > > > > The usual ISO 9660 producers on GNU/Linux write the CE entry as last > > > entry of System Use field or Continuation Area. So the problem does not > > > show up with their output. Nevertheless, Linux and libisofs obey the > > > specs whereas GRUB does not. > > > > > > As demonstration i crafted a small ISO, where the CE entry comes before > > > the NM entry which tells the Rock Ridge file name "RockRidgeName:x". > > > Linux shows the NM name, nevertheless: > > > $ sudo mount iso9660_early_ce.iso /mnt/iso > > > mount: /mnt/iso: WARNING: source write-protected, mounted read-only. > > > $ ls /mnt/iso > > > RockRidgeName:x > > > $ > > > > > > GRUB does not see the NM entry and thus shows the dull ISO 9660 name > > > (which is actually "ROCKRIDG.;1"): > > > $ ./grub-fstest iso9660_early_ce.iso ls / > > > rockridg > > > $ > > > > > > After the code change of my patch, i get: > > > $ ./grub-fstest iso9660_early_ce.iso ls / > > > RockRidgeName:x > > > $ > > > > > > A new code block in tests/iso9660_test.in verifies that the patched code > > > is in effect: > > > make check TESTS=iso9660_test > > > detects the old code state and shows that the new code still has the > > > capability to cope with endless CE loops. > > > > > > ------------------------------------------------------------------------- > > > How to create an ISO 9660 filesystem where CE is not the last SUSP entry > > > of a file's directory record: > > > > > > # Deliberately chosen names > > > iso=iso9660_early_ce.iso > > > # rr_path is longer than 8, mixed-case, with non-ISO-9660 character > > > rr_path=/RockRidgeName:x > > > > > > # A dummy file as payload > > > echo x >x > > > > > > # 250 fattr characters to surely exceed the size of a directory record > > > > > > long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789 > > > > > > # Create ISO with the payload file and attached fattr named > > > user.dummy . > > > # Make it small with the most restrictive ISO 9660 file name rules. > > > test -e "$iso" && rm "$iso" > > > xorriso -compliance no_emul_toc:iso_9660_level=1 \ > > > -padding 0 \ > > > -outdev "$iso" \ > > > -xattr on \ > > > -map x "$rr_path" \ > > > -setfattr user.dummy "$long_string" "$rr_path" -- > > > > > > # Cut out the NM field and the CE field from the directory record > > > # of $rr_path. The numbers were determined by looking at a hex dump. > > > dd if="$iso" bs=1 skip=37198 count=20 of=nm_field > > > dd if="$iso" bs=1 skip=37218 count=28 of=ce_field > > > > > > # Put them back in reverse sequence > > > dd conv=notrunc if=ce_field bs=1 seek=37198 of="$iso" > > > dd conv=notrunc if=nm_field bs=1 seek=37226 of="$iso" > > > > Patch set LGTM. Though I would prefer to hear Glenn's opinion on the test > > Thomas introduces because AIUI this [1] email presented some concerns. > > > > Glenn? > > > > Daniel > > > > [1] https://lists.gnu.org/archive/html/grub-devel/2023-03/msg00022.html > > I had partially written a response to the above referenced email a couple > weeks ago, but didn't sent it because it seemed unnecessary with the way > Thomas changed the test to work with or without extra trailing spaces. So > the concerns in the referenced email aren't a concern for this test patch. > > As for a more general concern, not really either. If the trailing spaces are > removed later on, tests will fail and then they can be fixed. To sum up what > I was going to say in response to the referenced email, grub-fstest is a > tool for running grub commands/code in user-space and has subcommands > corresponding for specifying what code to run. I haven't verified, but I > believe that in this case the grub "ls" command is creating the output, so > its likely that the command is what is producing the extra space, not > grub-fstest itself. Again, not something to worry about here, IMO. > > So patch #2, the test patch, in the series LGTM.
Glenn, thank you for detailed response. Then for both patches Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>... Thomas, thank you for fixing this issue. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
