Details

Reviewers

bcran
cem

Group Reviewers

manpages

Commits

rS351273: makefs: add msdosfs (FAT) support

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

Lint

Lint Skipped

Unit

Tests Skipped

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	Bump .Dd on commit
246	included in although this could probably be clarified, and out to be committed (referencing only ffs) independently first.
usr.sbin/makefs/makefs.c
77	alpha order
usr.sbin/makefs/msdos.h
45–46	alpha order
usr.sbin/makefs/msdos/Makefile.inc
5	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
2	rather different
usr.sbin/makefs/msdos/msdosfs_fat.c
1	modified sys/fs/msdosfs/msdosfs_fat.c

emaste mentioned this in D11197: msdosfs: expose and hide definitions/functions for makefs FAT fs support.Jul 25 2018, 2:29 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

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
195–196	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

emaste added subscribers: manu, eadler, cem.Jul 25 2018, 8:47 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.

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

emaste added a parent revision: D16526: msdosfs: header changes to support makefs.Jul 31 2018, 12:53 PM

It's possible to build makefs FAT support now:

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

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

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	Not UINT64?
95–96	why'd we lose this?
106–107	This rename seems spurious
139	gratuitous rename

emaste added inline comments.Aug 2 2018, 12:25 AM

usr.sbin/makefs/msdos.c
139	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

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

bcran mentioned this in D20562: The efifat files are no longer used: remove the code to build them..Jun 11 2019, 2:57 PM

freqlabs added a subscriber: freqlabs.Aug 15 2019, 8:03 PM

darkfiberiru_gmail.com added a subscriber: darkfiberiru_gmail.com.Aug 16 2019, 4:16 PM

emaste added inline comments.Aug 16 2019, 7:40 PM

usr.sbin/makefs/msdos.c
83	.maximum is a `long long`, not sure what the idea was before
95–96	not sure

cem added inline comments.Aug 17 2019, 1:31 AM

usr.sbin/makefs/msdos.c
83	Is there any reason for it to be a signed type? Not that it matters too much for this use.

freqlabs mentioned this in D21316: Expand description of image-size in makefs(8).Aug 18 2019, 3:25 PM

rebase

emaste added inline comments.Aug 19 2019, 9:40 PM

usr.sbin/makefs/msdos.c
95–96	It is defined in NetBSD's sys/fs/msdosfs/msdosfsmount.h

rebase and remove extraneous changes

emaste added a subscriber: smahadevan_freebsdfoundation.org.Aug 20 2019, 1:07 PM

emaste added inline comments.

usr.sbin/makefs/makefs.8
233–245	Done in rS351232, need to rebase this review.
usr.sbin/makefs/makefs.h
186–188	these should probably be in alpha order (as a separate commit)
usr.sbin/makefs/msdos.c
106–107	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`.

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	nit: sort alphabetically, one per line.

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

Closed by commit rS351273: makefs: add msdosfs (FAT) support (authored by emaste). · Explain WhyAug 20 2019, 6:20 PM

This revision was automatically updated to reflect the committed changes.

emaste added a commit: rS351273: makefs: add msdosfs (FAT) support.

Herald added a reviewer: manpages. · View Herald TranscriptAug 20 2019, 6:21 PM

makefs: add msdosfs support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 46180

usr.sbin/makefs/Makefile

usr.sbin/makefs/makefs.h

usr.sbin/makefs/makefs.8

usr.sbin/makefs/makefs.c

usr.sbin/makefs/msdos.h

usr.sbin/makefs/msdos.c

usr.sbin/makefs/msdos/Makefile.inc

usr.sbin/makefs/msdos/msdosfs_conv.c

usr.sbin/makefs/msdos/msdosfs_denode.c

usr.sbin/makefs/msdos/msdosfs_fat.c

usr.sbin/makefs/msdos/msdosfs_lookup.c

usr.sbin/makefs/msdos/msdosfs_vfsops.c

usr.sbin/makefs/msdos/msdosfs_vnops.c

makefs: add msdosfs supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 46180

usr.sbin/makefs/Makefile

usr.sbin/makefs/makefs.h

usr.sbin/makefs/makefs.8

usr.sbin/makefs/makefs.c

usr.sbin/makefs/msdos.h

usr.sbin/makefs/msdos.c

usr.sbin/makefs/msdos/Makefile.inc

usr.sbin/makefs/msdos/msdosfs_conv.c

usr.sbin/makefs/msdos/msdosfs_denode.c

usr.sbin/makefs/msdos/msdosfs_fat.c

usr.sbin/makefs/msdos/msdosfs_lookup.c

usr.sbin/makefs/msdos/msdosfs_vfsops.c

usr.sbin/makefs/msdos/msdosfs_vnops.c

makefs: add msdosfs support
ClosedPublic
Actions

Revision Contents
Changeset List