msdosfs: expose and hide definitions/functions for makefs FAT fs support

[emaste]

Summary

Additionally further adjusting #ifdefs similar to r319870

[emaste]

sys/fs/msdosfs/direntry.h

136

I think I got this one from comparing NetBSD and FreeBSD, but maybe I was overzealous. How does NetBSD use this header?

sys/fs/msdosfs/msdosfsmount.h

218

should this be #ifdef KERNEL?

[guest-svmhdvn]

sys/fs/msdosfs/direntry.h

136

I made this change because I've added a file usr.sbin/makefs/msdos/msdosfs_conv.c which reimplements a couple of these prototyped functions (winChksum, winSlotCnt, etc). Instead of wrapping the function declarations with #ifdef MAKEFS ... #else ..., I've prototyped them in an usr.sbin/makefs/msdos/msdosfs_extern.h file. Netbsd has _KERNEL || MAKEFS, but our sys/fs/msdosfs/direntry.h has different parameters for some of these reimplemented functions.

sys/fs/msdosfs/msdosfsmount.h

218

Yes, you're right. I'll update that.

[guest-svmhdvn]

Changed #ifndef MAKEFS to #ifdef _KERNEL.

[emaste]

A few inline comments; overall I think this looks good.

I suspect we want to make the DE_EXTERNALIZE changes first as a bugfix.

sys/fs/msdosfs/denode.h

197–198

This came in with NetBSD's FAT32 support

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/msdosfs/Attic/denode.h.diff?r1=1.23&r2=1.24&only_with_tag=MAIN&f=h

199–200

PR/32003: Brian Buhrow: msdosfs doesn't properly zero out high cluster data
on non-FAT32 msdos filesystems.

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/fs/msdosfs/denode.h.diff?r1=1.9&r2=1.10&only_with_tag=MAIN&f=h

sys/fs/msdosfs/msdosfs_lookup.c

661

can you explain this one?

690

we don't want to mix style & functional changes

762

From NetBSD Kill caddr_t; there will be some MI fallout, but it will be fixed shortly.

sys/fs/msdosfs/msdosfsmount.h

244

/* _KERNEL */

[guest-svmhdvn]

Removed style change, added comment for #ifdef

[guest-svmhdvn]

sys/fs/msdosfs/msdosfs_lookup.c

661

This came from here: http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/msdosfs/Attic/msdosfs_lookup.c.diff?r1=1.46&r2=1.47&only_with_tag=MAIN&f=h

Wed Mar 22 14:56:56 2000 UTC (17 years, 2 months ago) by jdolecek

createde(): if an error occurs, make sure to mark all modified directory
    slots as deleted - otherwise we would leave bogus slots in

This change actually seems to be redundant, as both in netbsd's and freebsd's code, we have a macro definition in sys/fs/msdosfs/msdosfsmount.h:

#define bptoep(pmp, bp, dirofs) \
    ((struct direntry *)(((char *)(bp)->b_data)	\
        + ((dirofs) & (pmp)->pm_crbomask)))

Thus, the line above, if (dirclust != MSDOSFSROOT) ... is also redundant, since we are just doing a bitwise AND twice.

Should I remove these redundant lines and revert the change?

[guest-svmhdvn]

Reverted DEEXTERNALIZE changes, refer to D11217 for bugfix

[guest-svmhdvn]

sys/fs/msdosfs/msdosfsmount.h

218

Actually, there seems to be another usr.sbin tool (amd) that depends on struct msdosfs_args, which is why I think I made this change to #ifndef MAKEFS. Before this patch, this struct was being exposed to all consumers of this header. I will need to revert it for building that tool.

[guest-svmhdvn]

Shouldn't expose locking macros to makefs

[emaste]

One final question, then I think this one is good to go in.

sys/fs/msdosfs/msdosfs_fat.c

1099–1115

What's the explanation for this one? AFAICT OpenBSD's kernel msdosfs_fat.c matches our existing (!MAKEFS) case, while the newly added #else here matches NetBSD's current kernel implementation.

[guest-svmhdvn]

sys/fs/msdosfs/msdosfs_fat.c

1099–1115

OpenBSD actually created usr.sbin/makefs/msdos/msdosfs_fat.c to override the functionality of their kernel implementation. This new file uses the same code as the newly added #else ... #endif code from the snippet above. The reason I made this change is in the following:

bp = getblk(DETOV(dep),
    frcn++,
    pmp->pm_bpcluster, 0, 0, 0);

DETOV(dep) will always be NULL in makefs.

However, I'm not sure if the way to go is to create a new msdosfs_fat.c in makefs like OpenBSD did or to keep it the way it is now. What do you think?

[emaste]

sys/fs/msdosfs/denode.h

273–276

We should sort these (no need to re-upload for just this).

[kib]

Honestly, I dislike the change. IMO, the msdosfs code should be copied to the userspace utility instead of making the hard-to-deal knot between kernel and userspace code.

Such coupling is very fragile, it will break on the next msdosfs change. Worse, the person who would do that change of course forget or is not aware of the need to check userspace build. Also, msdosfs changes would need to be adapted to the userspace, i.e. force anybody working with kernel driver to work on the makefs. BTW, the reverse is also true, you cannot change makefs without drilling into the kernel.

I already have very frustrating experience with similar coupling more than once. Most recent example was r305902 and r305903.

[emaste]

I would prefer that we share the common files, and have the side effect of allowing us to more easily test and debug the msdosfs code, but in the interest of expediency I've posted a rebased version of Siva's work that copies the files to userland -- D16438. We can take a careful look at deduplication later on.

[emaste]

fat.h deduped in D21346