Re: [Info-mtools] Bug: mformat does not format FAT32 correctly

[Pali Rohár]

On Sunday 23 September 2018 23:06:49 Alain Knaff wrote:
> Hi,
> 
> On 11/08/18 15:42, Pali Rohár wrote:
> > On Monday 04 December 2017 19:29:53 Pali Rohár wrote:
> >> 1) mformat is not able to format disk images without specifying C/H/S
> >> geometry. Nowadays there is no hard disk or SSD disk which use C/H/S
> >> geometry and therefore asking user for such thing does not make sense.
> 
> Actually, with physical disks, mtools asks the kernel for a geometry.

Yes, and kernel can return error (geometry not available). This happens
also for block devices (e.g SD card, or loopback).

And maybe same problem would be also for NVME disks.

> However, you're right: the issue is indeed relevant for image files,
> which have no innate geometry.

Yes, it is relevant also for image files.

> [...]
> > Hi! In attachment is a patch which automatically calculates C/H/S
> > geometry based on LBA Assist Translation formula just from the size of
> > disk. So formatting non-C/H/S disks (today *all* disks) via mformat is
> > automatic without need to specify C/H/S numbers. This applies also for
> > disk images.
> > 
> > This patch also adds support for detecting logical sector size of disk
> > and propagates it to FAT sector size. This is needed for a new Native 4K
> > disks which have sector size 4096 bytes (and not 512).
> > 
> > So with this patch (and previous one) is formatting Native 4K disk of
> > 16 TB to FAT32 easily, just with command:
> > 
> > $ mformat -F :: -i /dev/sdc
> > 
> 
> I looked at the patch, and it seems to contain more than bargained for :-)
> 
> Indeed, there seems to be debugging code left in: messages printed to
> stdout, or messages meant for final diagnostic being printed out right
> away before all available options (drive definitions) have been tried.

Hm.. there are no messages printed to stdout. And also there are no
debugging code. Those messages are printed to string buffer (sprintf)
and contains errors. In same way how other functions do it.

> Also, there is some puzzling "late capping" of sector_size which should
> be unneeded (ssize is already being capped at input), and which would
> introduce inconsistencies if for some reason it did indeed kick in.

No. ssize (= sector size) is no set correctly from block device. It is
set only when specified in config file or hardcoded for specific devices
(like floppy, etc.).

To get sector size of block device you need to call BLKSSZGET ioctl. And
this call is not called in mtools 4.0.18 (you can grep it).

It should be part of the get_block_geom() function which get disk
geometry. In past all disks had BLKSSZGET just 512, but today it is not
truth.

And without correct sector size of block device (BLKSSZGET) it would not
work correctly.

> Could you please review your patch, and only keep in those items that
> are actually relevant for the purpose at hand (C/H/S calculation and
> reading sector size), and resend it?
> 
> ... or, if these are indeed needed, maybe include a comment and/or
> explanation what they are doing?

I think that all parts in patch are relevant and needed. First one
updates code for reading disk geometry to read also sector size. Second,
function get_lba_geom add code which calculates LBA/CHS geometry -- it
supports file images and block devices (on linux you need special code
for every type; either stat() or BLKGETSIZE/BLKSSZGET). And the last
part changes error messages to reflect new code.

> Thanks,
> 
> Alain

-- 
Pali Rohár
address@hidden