Re: patch: MFSv3 support (libsa) for boot2 (i386)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

Hi,

Evgeniy Ivanov wrote:

> Here're patches to implement MINIX File System v3 support in libsa.
> Support for MFS is added to the boot2 for i386.

Several comments:

- I don't know about MINIX FS v3, but it looks similar to Linux Ext2fs.
  Is it difficult to share some code among them?

- Do you have any plan to add kernel support for the MINIX FS v3?
  If not, are changes outside sys/libsa necessary?
  (common/lib/libutil, libkern/xlat_mbr_fstype.c, sys/disk.h etc.)

- What environment do you test these loaders?
  Is there any tool to create/check these file system like
  e2fsprogs for Ext2fs?

- There are some "XXX should handle LARGEFILE" comments (that I put
  for ext2fs REV1 inode), but does MINIX FS have the similar member?
  It doesn't seem there are extra members in struct mfs_dinode.

- struct mfs_sblock seems to have many uint16_t and one char members.
  Probably it's better to put explicit padding and specify __packed
  to avoid unexpected machine dependent alignment restrictions.

- in sys/lib/libsa/mfsv3.c:mfsv3_i_bswap()
 - mdi_zone[] in  is unused? (untesed on big endian?)
 - (int16_t) cast to bswap16() is not necessary?
 - zone_t is typedef'ed as uint32_t. Shouldn't mdi_zone[] members
   also be swapped?

- in sys/lib/libsa/mfsv3.c:mfsv3_open()
 - fsmod and fsmod2 are used to pass module info for bootloaders
   to load. If currently no kernel support for MINIX FS,
   fsmod and fsmod2 should be left nothing.

- Copyright notice seems inconsistent:

>> + * Copyright (c) 2011
>> + *     Vrije Universiteit, Amsterdam, The Netherlands. All rights reserved.
>> + *
>> + * Author: Evgeniy Ivanov (based on ext2fs.c).

Are you sure that the copyright holder is not you but VU?

>> + * This code is derived from software contributed to The NetBSD Foundation
>> + * by Martin Husemann.

Is this still valid statement or just pasto?

+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS

Probably not TNF, but the author or copyright holders.

---
Izumi Tsutsui