mkfs.fat: Fix bad sector marking check

[etlow]

The check was for 512 byte sector number being more than actual number of sectors. Change it to block number being after number of blocks, and ignore blocks past end of last cluster. Also remove 512 byte hard sectors.

I am new to this, suggestions and criticisms welcome!

[etlow]

I noticed that some of the existing variables have a minimum size which is too small for the value. For example, value of mark_FAT_cluster has a type of unsigned int with a minimum size of 16 bits. I know it is normally compiled as 32 bits, so is it a problem and should I change the types?

Also, the bad blocks list is interpreted with a block size of 1024 bytes. Would it be better to use the sector size or cluster size instead, like mkfs.ext4 which uses the actual block size? If the fixed size of 1024 is good, I will add that to the -l documentation.

[pali]

Hello! Whole dosfstools code expects that int type has at least 32-bit range. I know that C standard specifies minimal 16-bit depth, but I do not know if it makes sense to start patching whole code as all modern (linux) systems are 32 or 64-bit where int type is always 32-bit.

Ad bad block size, it should be 1024 bytes long by default as this is the size used by badblocks tool. I thought that I mentioned it in manpage, if not then probably I forgot to push this change into master branch. I will look at it.

[pali]

Also, please put your name and your valid email address into git commits. I'm not happy with merging or accepting changes from anonymous people without any contact option. dosfstools is not anonymous project, it is key tool in lot of linux distributions due to EFI and FAT usage.

[etlow]

Thanks for the reply! I was looking through the code and also saw off_t, but I realised that this is not a problem on 32-bit systems due to AC_SYS_LARGEFILE being used.

As of now, the bad blocks block size is hardcoded to 1024 in mkfs.fat. I wouldn't mind making it variable, though a problem would be deciding whether to use sector or cluster size. Also, I noticed a minor typo in the first line of mkfs.fat.8.in though it doesn't affect the output.

I have added my name and also tried to minimise the indentation changes and leave int alone.

I just happened to read https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view, do you think it is a good idea to re-indent all the files and do this?

[pali]

Thanks for the reply! I was looking through the code and also saw off_t, but I realised that this is not a problem on 32-bit systems due to AC_SYS_LARGEFILE being used.

Beware that AC_SYS_LARGEFILE is for allowing user to use 64-bit off_t type. It is not for forcing. So if some code requires 64-bit type then please use long long type, not off_t. Use off_t only for arguments passed to system functions which takes off_t.

As of now, the bad blocks block size is hardcoded to 1024 in mkfs.fat. I wouldn't mind making it variable, though a problem would be deciding whether to use sector or cluster size.

Default value should be always what badblocks program use by default.

do you think it is a good idea to re-indent all the files and do this?

No, please do not reformat files. It does not bring any value. You probably have some code style, but other people have different... and for example somebody in next years can come up with the same idea to reformat everything to different style. I just do not see any value in it...

[etlow]

The current variables using off_t are passed into lseek after being multiplied by sector size or block size. As such, the lseek parameter will overflow before those variables overflow. I guess this case is ok, I didn't touch it.

I agree the default block size should be the same as badblocks which is 1024. Does that mean it has to be hardcoded to 1024, because the default sector size and cluster sizes are unlikely to be 1024? I can think of only three options: hardcode to 1024, follow sector/cluster size, or maybe a separate command line argument which seems inelegant. I would lean towards whatever sector size mkfs.fat has decided to format, even though that is probably 512 and not 1024.

badblocks man page:

Important note: If the output of badblocks is going to be fed to the e2fsck or mke2fs programs, it is important that the block size is properly specified, since the block numbers which are generated are very dependent on the block size in use by the filesystem. For this reason, it is strongly recommended that users not run badblocks directly, but rather use the -c option of the e2fsck and mke2fs programs.

Options -b block-size Specify the size of blocks in bytes. The default is 1024.

I would normally not suggest re-indenting, however the current indentation is a mix of tabs and spaces. The current indentation being used is mainly 4 spaces, and 8 spaces (double indent) being replaced by 1 tab. I remember seeing someone here saying that they could not get their editor to handle the combination of spaces and tabs. The two main styles of indentation I see is either 4 spaces or tabs. Even though I would prefer 4 spaces, I don't mind using tabs, as long as it is consistent.

[pali]

The current variables using off_t are passed into lseek after being multiplied by sector size or block size. As such, the lseek parameter will overflow before those variables overflow. I guess this case is ok, I didn't touch it.

There should be added overflow checks. Meaning that multiplication of off_t value by some sector size still can be represented in off_t.

I agree the default block size should be the same as badblocks which is 1024. Does that mean it has to be hardcoded to 1024, because the default sector size and cluster sizes are unlikely to be 1024? I can think of only three options: hardcode to 1024, follow sector/cluster size, or maybe a separate command line argument which seems inelegant. I would lean towards whatever sector size mkfs.fat has decided to format, even though that is probably 512 and not 1024.

Maybe it makes sense to have an option for changing it if default value 1024 is not a good option for user.

And about style, I think you can use spaces but just for your changes, not reformatting unrelated code.

[pali]

@etlow Have you looked at it?