natevw / fatfs

Standalone FAT16/FAT32 filesystem implementation in JavaScript
47 stars 13 forks source link

Changes in "util-linux" (blkid) -> LABEL vs. LABEL_FATBOOT #30

Open flipsa opened 5 years ago

flipsa commented 5 years ago

Hey @natevw

I just ran into a problem with software that relies on fatfs. See this Xen Orchestra issue.

I am not sure if you are aware of it, but util-linux > 2.33-rc1 now uses "LABEL_FATBOOT" instead of "LABEL" for labels written to the boot sector of a disk. Is it possible to change this in fatfs, or maybe set both LABEL and LABEL_FATBOOT (to the same value)?

natevw commented 5 years ago

Hi @flipsa, thanks for this report though I must admit I'm a bit puzzled. The only FAT-related field might be the VolLab but I don't recall that is even being exposed by this library and generally this just works on an existing (pre-formatted) partition. I wonder if it's another component that needs to handle the volume's partition table setup differently?

flipsa commented 5 years ago

Hey @natevw

Maybe I misunderstood how fatfs works, or how it's being used by Xen Orchestra, but after skimming their code I was under the assumption that they write the label into VolLab. I'm not too familiar with the XO codebase, but I think this is the relevant piece of code: https://github.com/vatesfr/xen-orchestra/blob/master/packages/xo-server/src/fatfs-buffer.js

natevw commented 5 years ago

Yes, it looks like they do use one of this libraries structs directly to format the volume from xo-server itself, and set the VolLab in the boot header.

The summary at https://lkml.org/lkml/2017/10/4/512 is helpful and clarifies that the boot structure value is usually not used. I'm assuming that would be the LABEL_FATBOOT version you mention.

In order to set the other entry which I presume is the LABEL variable, it looks like XO would need to also initialize a special file entry with a "Volume Label" flag attribute set when they format the volume.

This library doesn't usually do anything with this kind of entry except to ignore it when iterating through through a directory listing. However, studying the file entry handling code it looks like it might be possible to set the FREG (octal 0100000) permissions mode bit on a file to set that special Volume Label flag. I haven't tested this though.

natevw commented 5 years ago

Actually, correction:

The FREG flag fatfs/struts.js corresponds to S_IFREG in <sys/stat.h>. That is, it indicates that the entry is a "regular" file and so it is usually already set for files.

From the public interface of fatfs it looks like this flag is made inaccessible via the public interface because fchmod whitelists out the mode argument passed to it and preserves the special directory/entry flags. The most straightforward way might be for the XO code to create this special directory entry itself using the [internal] fatfs struct definitions which it is already importing from to initialize the volume boot record.

julien-f commented 4 years ago

@natevw Hey I'm looking into it but I have to say that I don't see how to do it, would it possible for you to give me a small example? :slightly_smiling_face:

natevw commented 4 years ago

@julien-f Sorry been busy with other projects and I have to admit I'm myself rusty on the details of this library.

Basically you would need to do something like what dir.addFile would do, creating a file in the root directory chain but with a special bit set. The change is that after the current call to _updateEntry you would tweak the mode, something like:

_updateEntry(vol, mainEntry, {firstCluster:fileCluster, size:0, ctime:true,_touch:true});
mainEntry.Attr.volume_id = true;
// … proceed …

Or more simply, create a file in the root directory of the volume using the normal API and then do what dir.updateEntry would do when newStats.mode = 0 [I think]. The ultimate goal in either case is to trigger a true value for entry.Attr.volume_id in/after _updateEntry so that the special bit gets recorded when the entry is serialized to disk.

Does that help a bit?

Ultimately, since iirc the original XO code where this is getting used is formatting a brand new volume, much more assumptions could be made and it might be possible to sort of "hardcode" all the offsets/entries a bit more since this would be the only directory entry to worry about at that point.

This is unnecessarily complicated because the public API of fatfs prevents this in its attempt to be compatible with the real node.js filesystem API. Specifically fs.fchmod always sets the FREG bit on regular files, whereas for the special volume name "file" it would need to stay cleared. (fs.open seems not to apply mode at all which is probably a bug…)

natevw commented 4 years ago

Taking a step back:

I suspect it would be helpful to add a public "format volume" helper method that does everything needed here? That doesn't totally solve the general case (i.e. wouldn't allow changing the label later, etc.) but might help XO be less dependent on the gory internals here.

This would be a top-level API, sibling to the current fatfs.createFileSystem which for clarity creates an instance for accessing an existing filesystem. Something like fatfs.formatVolume taking whatever FAT parameters would be appropriate including a volume label.

Specifically, this would move the work started at https://github.com/vatesfr/xen-orchestra/blob/ad58f6a14795feebe3caf9b1db5f05b593586f6d/packages/xo-server/src/fatfs-buffer.js#L27 into a public method on this library. But make it a bit more generic: using a volume driver rather than initializing own buffer, and un-hardcoding configuration stuff using on driver.sectorSize+driver.numSectors instead. Maybe letting user control e.g. FAT12/16/32. And of course the big issue here of storing the volume label as that special file entry…

julien-f commented 4 years ago

Sorry been busy with other projects and I have to admit I'm myself rusty on the details of this library.

No problem, I'm also rather busy, I'll take a look at your answer in the coming weeks when I have more time, thank you so much! :slightly_smiling_face:

andrzejressel commented 4 years ago

So yesterday and today I was researching this and I've discovered few interesting things:

I've managed to add hacky support by replacing

    if ('firstCluster' in newStats) {
        entry.FstClusLO = newStats.firstCluster & 0xFFFF;
        entry.FstClusHI = newStats.firstCluster >>> 16;
        entry._firstCluster = newStats.firstCluster;
    }

with

    entry.Attr.volume_id = true

    if ('firstCluster' in newStats) {
        entry.FstClusLO = newStats.firstCluster & 0xFFFF;
        entry.FstClusHI = newStats.firstCluster >>> 16;
        entry._firstCluster = newStats.firstCluster;
    }

    entry.FstClusLO = 0
    entry.FstClusHI = 0

in dir.js

UPDATE1: I see there is one issue however: label is in uppercase and I think cloud-init requires is to be lowercase

UPDATE2: Fixed:

replace

var _snInvalid = /[^A-Z0-9$%'-_@~`!(){}^#&.]/g;
name = name.toUpperCase().replace(/ /g, '').replace(/^\.+/, '');

with

var _snInvalid = /[^A-Za-z0-9$%'-_@~`!(){}^#&.]/g;
name = name.replace(/ /g, '').replace(/^\.+/, '');

in helpers.js

andrzejressel commented 4 years ago

~So I see some issues with supporting lowercase characters. From what I understand names by default must be uppercase and lowercase letters can be used only(?) in VFAT file names. Unfortunatelly util-linux does not support reading label from them: https://github.com/karelzak/util-linux/blob/f0ca7e80d7a171701d0d04a3eae22d97f15d0683/libblkid/src/superblocks/vfat.c#L162~

Lower/Upper case is not an issue, because cloud-init works with both