Page MenuHomeFreeBSD

makefs: add msdosfs support
ClosedPublic

Authored by emaste on Jul 25 2018, 1:52 PM.

Details

Summary

Rebased patch from Siva Mahadevan. Unlike previous work this duplicates the msdosfs kernel files in usr.sbin/makefs/msdos as requested in D11197.

I would prefer that we share common files between kernel and userland, but in the interest of expediency am fine with pursuing this approach for now, perhaps followed by careful deduplication.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste created this revision.Jul 25 2018, 1:52 PM
emaste added inline comments.Jul 25 2018, 2:04 PM
usr.sbin/makefs/ffs/buf.h
48–50 ↗(On Diff #45825)

Changes in this file seem to be whitespace/style; if we're going to include them, commit them separately.

usr.sbin/makefs/makefs.8
38 ↗(On Diff #45825)

Bump .Dd on commit

246 ↗(On Diff #45825)

included in although this could probably be clarified, and out to be committed (referencing only ffs) independently first.

usr.sbin/makefs/makefs.c
78 ↗(On Diff #45825)

alpha order

usr.sbin/makefs/msdos.h
46–47 ↗(On Diff #45825)

alpha order

usr.sbin/makefs/msdos/Makefile.inc
4 ↗(On Diff #45825)

I think this path should not be included

usr.sbin/makefs/msdos/denode.h
1 ↗(On Diff #45825)

Copy of sys/fs/msdosfs/denode.h with #ifdef KERNEL block removed. This one is likely easily shared.

usr.sbin/makefs/msdos/direntry.h
1 ↗(On Diff #45825)

Copy of sys/fs/msdosfs/denode.h with #ifdef KERNEL block changed.

usr.sbin/makefs/msdos/fat.h
1 ↗(On Diff #45825)

sys/fs/msdosfs/fat.h modulo _KERNEL

usr.sbin/makefs/msdos/msdosfs_conv.c
1 ↗(On Diff #45825)

rather different

usr.sbin/makefs/msdos/msdosfs_fat.c
1 ↗(On Diff #45825)

modified sys/fs/msdosfs/msdosfs_fat.c

emaste updated this revision to Diff 45840.Jul 25 2018, 6:01 PM
  • Exclude buf.h changes
  • Rebase on newly copied sys/fs/msdosfs files
emaste marked 7 inline comments as done.Jul 25 2018, 6:07 PM
emaste added inline comments.Jul 25 2018, 7:27 PM
usr.sbin/makefs/msdos/msdosfs_conv.c
1 ↗(On Diff #45825)

Our kernel version of this file sys/fs/msdosfs/msdosfs_conv.c corresponds to NetBSD sys/msdosfs/msdosfs_conv.c 1.25
https://github.com/NetBSD/src/commit/9ee2678cd5ee833080978bd635bb22fbf0ef7839

I'm not sure where the version of this file in this review comes from (specifically, I can't seem to find a copy of this with a 1994 Copyright).

For reference, versions from other BSDs:

NetBSD (sys/fs/msdosfs/msdosfs_conv.c)

/*      $NetBSD: msdosfs_conv.c,v 1.17 2016/06/30 09:34:01 nonaka Exp $ */

/*-
 * Copyright (C) 1995, 1997 Wolfgang Solfrank.
 * Copyright (C) 1995, 1997 TooLs GmbH.
 * All rights reserved.
 * Original code by Paul Popelka (paulp@uts.amdahl.com) (see below).

OpenBSD (sys/msdosfs/msdosfs_conv.c)

/*      $OpenBSD: msdosfs_conv.c,v 1.19 2015/10/23 10:45:31 krw Exp $   */
/*      $NetBSD: msdosfs_conv.c,v 1.24 1997/10/17 11:23:54 ws Exp $     */

/*-
 * Copyright (C) 1995, 1997 Wolfgang Solfrank.
 * Copyright (C) 1995, 1997 TooLs GmbH.
 * All rights reserved.
 * Original code by Paul Popelka (paulp@uts.amdahl.com) (see below).

OpenBSD (usr.sbin/makefs/msdos/msdosfs_conv.c)

/*      $OpenBSD: msdosfs_conv.c,v 1.1 2016/10/18 17:05:30 natano Exp $ */
/*      $NetBSD: msdosfs_conv.c,v 1.24 1997/10/17 11:23:54 ws Exp $     */

/*-
 * Copyright (C) 1995, 1997 Wolfgang Solfrank.
 * Copyright (C) 1995, 1997 TooLs GmbH.
 * All rights reserved.
 * Original code by Paul Popelka (paulp@uts.amdahl.com) (see below).

FreeBSD (sys/fs/msdosfs/msdosfs_conv.c)

/* $FreeBSD$ */
/*      $NetBSD: msdosfs_conv.c,v 1.25 1997/11/17 15:36:40 ws Exp $     */

/*-
 * SPDX-License-Identifier: BSD-4-Clause
 *
 * Copyright (C) 1995, 1997 Wolfgang Solfrank.
 * Copyright (C) 1995, 1997 TooLs GmbH.
 * All rights reserved.
 * Original code by Paul Popelka (paulp@uts.amdahl.com) (see below).
emaste added inline comments.Jul 25 2018, 8:45 PM
usr.sbin/makefs/msdos/msdosfs_conv.c
194–195 ↗(On Diff #45840)

This change was introduced in NetBSD msdosfs_conv.c 1.17, so this file appears to be a modified copy of the latest available in NetBSD.

Compared to that version, this has:

  • some functions reordered
  • unix2winfn added from somewhere (this may explain the modified copyright information)
  • use of C99 types (uint16_t instead of u_int16_t)
  • miscellaneous header changes
  • dos2unix[] array removed
  • size_t instead of int for sizes
  • some UTF-related changes
cem added a comment.Jul 26 2018, 10:34 PM

I'm not really a fan of this duplicating code approach. Despite the pain that building code in both the kernel and userspace can occasionally cause, I think it's worth it, at least here. How much work does msdosfs really see in 2018? kib's example from the other revision is almost two years old. While I'm sure there have been more recent examples, I don't think the degree of pain is so high as to overcome the alternative cost of maintaining duplicate code.

emaste updated this revision to Diff 46051.Jul 30 2018, 9:48 PM

Updated patch that shares kernel headers, based on part of D11197

It's possible to build makefs FAT support now:

  1. apply D16526 (header modifications)
  2. copy msdosfs_fat.c and msdosfs_lookup.c from sys/fs/msdosfs to usr.sbin/makefs/msdos
  3. apply D16438

I propose that we proceed with this approach, and subsequently update the kernel msdosfs support and deduplicate.

cem added a comment.Aug 1 2018, 11:03 PM

This is a pretty big patch. At least some of this appears to be refactoring. Can that be broken out and committed separately? Any other small pieces possible to break out would help, too.

It's still unclear to me why we're copying files from the kernel.

I'm not sure if we lost Unicode support with this change or not, but msdos with LFN supports 255 UCS-2 character filenames (i.e., BMP only Unicode).

usr.sbin/makefs/msdos.c
83 ↗(On Diff #46051)

Not UINT64?

95–96 ↗(On Diff #46051)

why'd we lose this?

105 ↗(On Diff #46051)

This rename seems spurious

139 ↗(On Diff #46051)

gratuitous rename

emaste added inline comments.Aug 2 2018, 12:25 AM
usr.sbin/makefs/msdos.c
139 ↗(On Diff #46051)

hrm, not sure how Siva ended up with this change; in any case it's not in NetBSD so will revert
http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.sbin/makefs/msdos.c?annotate=1.20&only_with_tag=MAIN

emaste updated this revision to Diff 46180.Aug 2 2018, 1:17 PM

Undo renames pointed out by @cem

emaste marked 2 inline comments as done.Aug 2 2018, 1:18 PM
emaste added a subscriber: bcran.Sep 17 2018, 9:20 PM
bcran added a reviewer: bcran.Nov 28 2018, 7:40 PM
emaste added inline comments.Aug 16 2019, 7:40 PM
usr.sbin/makefs/msdos.c
83 ↗(On Diff #46051)

.maximum is a long long, not sure what the idea was before

95–96 ↗(On Diff #46051)

not sure

cem added inline comments.Aug 17 2019, 1:31 AM
usr.sbin/makefs/msdos.c
83 ↗(On Diff #46051)

Is there any reason for it to be a signed type? Not that it matters too much for this use.

emaste added inline comments.Aug 19 2019, 9:40 PM
usr.sbin/makefs/msdos.c
95–96 ↗(On Diff #46051)

It is defined in NetBSD's sys/fs/msdosfs/msdosfsmount.h

emaste updated this revision to Diff 61040.Aug 20 2019, 1:06 PM

rebase and remove extraneous changes

emaste added inline comments.
usr.sbin/makefs/makefs.8
230–242 ↗(On Diff #61014)

Done in rS351232, need to rebase this review.

usr.sbin/makefs/makefs.h
186–187 ↗(On Diff #61014)

these should probably be in alpha order (as a separate commit)

usr.sbin/makefs/msdos.c
105 ↗(On Diff #46051)

Indeed, I'm not sure if @smahadevan_freebsdfoundation.org perhaps ended up with an older version of the NetBSD code in this change? In any case, this should be undone, assuming the current NetBSD code has msdos_opt.

imp added a comment.Aug 20 2019, 4:36 PM

one last nit.
Still object to copying, but since no progress on de-dupe has been made and there's been a 2 year lag, I accept it's the last bad choice to move this forward.

usr.sbin/makefs/Makefile
11 ↗(On Diff #61040)

nit: sort alphabetically, one per line.

cem accepted this revision.Aug 20 2019, 4:40 PM

I still don't like the duplication, but otherwise can't object. (My approval is contingent on Warner's remarks.)

This revision is now accepted and ready to land.Aug 20 2019, 4:40 PM
This revision was automatically updated to reflect the committed changes.