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