6964 mkfs_pcfs should detect GPT partition size and fat type for it

Review Request #190 — Created May 11, 2016 and submitted

tsoome
illumos-gate
6964
894e337...
general

6964 mkfs_pcfs should detect GPT partition size and fat type for it

mkfs on partition sizes 33MB (to get fat32), 32MB & 10MB to get fat16 and 1MB to get fat12.
Verified the result with fstyp -v.

I have used this updated mkfs in usbgen script (to create bootable usb images with distro constructor) as:

mkfs -F pcfs -o b=system ${rs0devs}

  • 0
  • 0
  • 12
  • 1
  • 13
Description From Last Updated
tsoome
rm
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/Makefile (Diff revision 1)
     
     
    Sorry, this is rather nitty, but would you mind lining this up with the rest?
  3. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
    The comment could be cleaned up a little bit. How about something like:
    
    'Extracts information about a disk path in dn. We need to return both a file descriptor and the device's suffix. Secondarily, we need to detect the FAT type and size when dealing with GPT partitions.
  4. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
    What was the error? If this happens in the field we'll have no leads to go on. Probably want a strerror of the VT_* error if possible.
    1. There is no strerror for VT_*, but I did "steal" idea from newfs/fsck nevertheless.

  5. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
     

    So, do you have a good sense of what the different error codes are supposed to mean here? It's not clear when why in some cases we exit 1 vs. 2.

    1. No, not really. And quick search does not reveal anything. However, as value 1 seems to be used just for usage(), and 2 for various IO errors (but not always), I replaced 1 with 2...

  6. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
    Why is there a leading \n here? Also, the format of these errors seems a bit different form others given that we're leading with the suffix.
    1. Ah, I got it carried over from initial check from open_and_seek(). Hopefully its better now.

  7. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
    Where does the magic number 7 come from?
    1. minor numbers for slices; a == 0 .. g == 6, and for GPT, wd (whole disk) is 7. Note for SMI/VTOC 7 is h, which has the same meaning; except on x86, where p0 is q == 16. The full list is in usr/src/uts/common/io/cmlb.c.

      The unfortunate issue is, it is seemingly "random" number... I'll add the comment about :wd node there.

  8. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     

    Can you explain a bit more about why this branch exists?

    1. The current mkfs is built assuming FAT16 as default, and only option to change this default is to use mkfs -F pcfs -o fat=32 or fat=12. The motivation for this whole update is from need to support EFI System partition, and the fact that apparently even if FAT12/FAT16/FAT32 all should be supported by UEFI spec., people have reported seeing issues with non FAT32 setups. Therefore, to have best support for user created system partition, mkfs attempts to prefer FAT32 over FAT16 over FAT12. The command line option is preferred over automatic detection, allowing to set custom value - so we are not breaking anything either.

  9. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     
    Rather than 'is using' how about uses or has
  10. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     

    It's probably worth commenting a bit more about the logic that's going on here or rather how it's variable on FAT type, etc. It sounds like the assumption depending on which FAT type to take is based entirely on looking at the size of this, but I'm not sure.

    1. Yes, I did add the comment. This type detection is based on checks performed in compute_cluster_size(), which is called later when initial setup is done. Also as the named function assumes hdd will not have fat12, I was in need to set SPC for case of fat12.

  11. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 1)
     
     

    What is SPC an abbreviation for? Presumably the comment should define it. Is this space?

    1. Sectors per cluster. The same abbreviation is used all over the source unfortunately, except the line 86 where the variable for user provided value is declared... I updated this comment nevertheless, as also I noticed the next line is exceeding 80 symbols...

  12. 
      
tsoome
tsoome
tsoome
rm
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 3)
     
     

    This should be 'device does not support the ioctl'.

  3. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 3)
     
     

    The break statement is incorrectly indented.

  4. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 3)
     
     

    The break statement is incorrectly indented.

  5. 
      
tsoome
rm
  1. Ship It!
  2. 
      
marcel
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 4)
     
     

    As a little cleanup, the "fd =" part should be removed.

  3. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 4)
     
     

    It looks like close(-1) might be called here. If that's really possible, I suggest to avoid that.

    1. I think it should not happen, but, instead of guessing, I did fix it (and other cases + one leak).

    2. Yes, you are right. It was not possible to call close(-1) there.

  4. 
      
tsoome
marcel
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 5)
     
     

    This initialization is not needed.

    1. True. And removed if (fd > -1).

  3. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 5)
     
     

    I suggest to move this below the following lines:

    if (Outputtofile)
    return (...);

    The you could save one "if (!Outputtofile)".

    1. yep it actually seems quite reasonable.

  4. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 5)
     
     

    It is not needed to close(2) files before the exit(2) is called (just a note, feel free to leave the close(2) call here, if you want).

    1. Yes I know, exiting app will close them etc. I personally like the idea about keeping track of the allocation-release logic, so when someone will
      decice change those exit() calls, the close() is there for reminder...

  5. 
      
marcel
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 5)
     
     

    Is it okay to move the stat_actual_disk() call after the "if" at line 2283? The stat_actual_disk() can change the suffix value...

    1. there seems to be some confusion about line numbers, seems like old diff version?

    2. Please ignore my comment. :-)

  3. 
      
tsoome
rm
  1. I think the updates seem reasonable. Thanks Toomas.
  2. 
      
marcel
  1. Ship It!
  2. 
      
yuripv
  1. 
      
  2. usr/src/cmd/fs.d/pcfs/mkfs/mkfs.c (Diff revision 6)
     
     

    How do we know that? You don't seem to check stat_actual_disk() result.

    1. stat_actual_disk() will exit(2) if the file does not exist. It has pile of other issues however - its assuming the strdup() will succeed, but I would classify this for different bug.

    2. Got it.

  3. 
      
yuripv
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...