Re: Seventh Edition(V7) filesystem support.

[christos%astron.com@localhost (Christos Zoulas)]

In article <20110617.220043.653026525707362873.uch%vnop.net@localhost>,
UCHIYAMA Yasushi  <uch%vnop.net@localhost> wrote:
>I have prepared patch for 5.99.53.
>ftp://ftp.netbsd.org/pub/NetBSD/misc/uch/ v7fs-5.99.53.patch and
>v7fs-5.99.53.tar.gz
> 
>I would like to commit this. Any objection? Review welcome.
>

general:
- functions that are only used in a single file should be declared static
- put all the forward declarations on top; actually if you make local functions
  static and you order them correctly you can delete a lot of the forward
  decls.
- __attribute__((unused)) -> __unused
- there are some lines > 80 columns.
- variables should not start with __
- you have minor whitespace issues '=value' instead of '= value' for example.
- error reporting to stderr; use more err(3), warn(3) when appropriate,
  perror(3) is ancient.
- should we be using the standard progress function ftp and progress are using
  maybe move that to libutil?
- usage() should be __attribute__((__noreturn__)), perhaps we need a __noreturn
  in cdefs.h, instead of abusing __dead
- it is traditional for the usage string to list the flags instead of saying
  [ fs options ].
- perhaps __packed instead of __attribute__


fsck:
- fsck programs have various FSCK_EXIT values that are obeyed by the rc
  scripts, so don't just EXIT_FAILURE.
- error reporting from fsck should be done to stderr, look at the functions
  the other fsck programs use to interact with the user.

mount:
- MNT_GETARGS could return the endianness of the mounted filesystem

newfs:
- why warnx() + exit()? errx()?

kernel:
- what is the use of DPRINTF("")?
- there are some stray printfs() instead of DPRINTF's.


Thanks for doing all this work, looks great!

christos