Additionally further adjusting #ifdefs similar to r319870
Unit Tests Skipped
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.
Yes, you're right. I'll update that.
A few inline comments; overall I think this looks good.
I suspect we want to make the DE_EXTERNALIZE changes first as a bugfix.
This came in with NetBSD's FAT32 support
PR/32003: Brian Buhrow: msdosfs doesn't properly zero out high cluster data on non-FAT32 msdos filesystems.
can you explain this one?
we don't want to mix style & functional changes
From NetBSD Kill caddr_t; there will be some MI fallout, but it will be fixed shortly.
/* _KERNEL */
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?
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.
One final question, then I think this one is good to go in.
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.
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?
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.
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.