Other than the commit title saying 'excludes fat16' rather than 'excludes fat32', it looks fine to me, and does everything it's supposed to do. I don't know if commit titles can be edited, I don't keep any code on github right now. If it can't easily be changed, I would just ignore it.
As a side note, firasuke's issue (https://github.com/landley/toybox/issues/519) can now be closed. On Fri, Nov 8, 2024 at 10:15 PM Rob Landley <r...@landley.net> wrote: > > On 11/8/24 10:51, Kana Steimle wrote: > > I realized after I sent my last email to you that it didn't go to the > > mailing list because I didn't re-add it to who I'm replying to. Do I > > forward it to the mailing list, or just leave it alone? > > We continue on from where we are. > > For example, I pushed my local tree to github to unbreak Elliott because > he pulls on fridays and needed the "QUIET" fix for his llvm toolchain, > but had your patch applied here without the extra file and pushed that > up without remembering to revert it first, so I tried to do a quick fix > (before seeing this) and got it wrong. So I come up with a patch on top > to undo that. > > (I may not be at my best right now. It's been a stressful few days.) > > >> No idea what gmail's up to these days. > > I've wanted to switch off of gmail and self-host a mail server instead, > > but the last time I tried that I could only receive emails and not send > > them. I'm just sticking with what works for now. > > > >> Your new test fails because there's no "fat32.bz2" file. (I'm assuming > >> you have one?) > > > > I included instructions in the patch on how to create the fat32.bz2 file > > in case you didn't want to trust binary files created by strangers on > > the internet (we all know how that went down with xz), > > I did: > > truncate -s 1m fat32.img && mkdosfs -F32 -I -nmyfat32 fat32.img > > But the UUID didn't match (and I didn't spot where to specify that in > the man page), and the host /sbin/blkid was outputting two extra fields > (LABEL_FATBOOT and BLOCK_SIZE) so I wasn't entirely certain what the > goal here was... > > > but if you don't > > care about that and just want a file you can drop in, I've attached it > > for you. > > It's small and not executed, if you can manage an exploit when we do > nothing but bunzip it (with my code) and run blkid on it (with my code) > then the problem is my code. :) > > >> The if (*type=='v') test is ok for distinguishing between "we just > >> checked for 'vfat' and 'iso9660' so which of the two was it", but in > >> code that's run for every filesystem I'm a lot less comfortable with > >> that. > > > > I was kind of iffy on the (*type=='v') test as well. Part of me thought I > > should replace it with a (!strcmp(type, "vat")) instead since I was > > moving the line out of the check for the full type names, but I guess > > that slipped my mind. If you think your method of checking !FLAG(U) > > is more efficient though, go for it. > > It's roughly equal, but my concern wasn't efficiency, it was potential > mistaken identity. Don't want to reenact the plot of "the man who knew > too little" by leaving sharp edges in the codebase. > > >> - if (*type=='v') show_tag("SEC_TYPE", "msdos"); > >> + if (*type=='v' && !FLAG(U)) show_tag("SEC_TYPE", "msdos"); > > I added the test `&& fstypes[i].magic_len == 4` in order to differentiate > > between fat12/16 and fat32, since both share the "vfat" type name. It's > > admittedly a bit arbitrary, but iirc it was the first clear difference > > between > > their structures in fstypes. I would instead do: > > - if (*type=='v') show_tag("SEC_TYPE", "msdos"); > > + if (*type=='v' && fstypes[i].magic_len == 4 && !FLAG(U)) > > show_tag("SEC_TYPE", "msdos"); > > I guess that's on me for not leaving a comment to explain why that test > > was there. > > Length of the MAGIC matching, not the fs name, the test is to _exclude_ > fat32, not include it. Also, we're already _inside_ !FLAG(U) the extra > test is !FLAG(L) > > Sigh. > > Ok, does commit 2045c952145e look ok to you? > > Thanks, > > Rob _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
