Subject: Re: scan_ffs from OpenBSD ported and improved.
To: Juan RP <juan@xtrarom.org>
From: Julio M. Merino Vidal <jmmv84@gmail.com>
List: tech-userlevel
Date: 06/06/2005 16:05:48
On Mon, 2005-06-06 at 14:12 +0200, Juan RP wrote:
> On Mon, 06 Jun 2005 14:06:48 +0000
> "Julio M. Merino Vidal" <jmmv84@gmail.com> wrote:
> 
> > On Mon, 2005-06-06 at 13:44 +0200, Juan RP wrote:
> > > Hi, finally I've fixed all problems I had reported in my previous email
> > > "Porting scan_ffs from OpenBSD", the following is the example of
> > > this utility to find FFSv2 partitions with different blocksizes:
> > [...]
> > > Please test and review, I'd want to commit this on the next days if there
> > > isn't any objection.
> > 
> > >From a very quick review, I see:
> > - Auxiliary functions are not static.
> 
> Should be declared as static? 

Yes.  static functions are "private" to the source file they are
declared in.

> > - Inconsistent spacing: there are some commas with a space after them,
> >   while others don't have it; there are multiple operators without
> >   spaces around them while others have it; etc.
> >   See /usr/share/misc/style.
> 
> For operations like (blk+(n/512)*foo), I wanted to leave them without spaces.

But it's not according to the style.  I personally don't like some other
things in it, but you must comply to them.  (Dunno if such an exception
in this case is OK; I'll let it to someone else.)

> > - Functions that return values which are discarded should be prefixed
> >   with (void).  I.e., (void)printf("blah");
> 
> Ok!
> 
> > - Options in getopt should be sorted alphabetically.
> 
> Ok!
> 
> > - Most NetBSD utilities don't have '-h' for help, while this one does.
> 
> Should "-h" be removed then?

Yes, I think so.  Though wait a bit to see if someone else replies.

> > - Where are the patches to modify the build infrastructure to build the
> >   utility?
> 
> I haven't added these patches, because they are trivial to add.

They are trivial, but you might be missing some detail.  If you post
them, there will be less chances to forget something.

> > - And the manual page?
> 
> I'll write the manual page tomorrow, I'd want to explain its current state
> finding FFSv1 partitions.

OK, thanks.

-- 
Julio M. Merino Vidal <jmmv84@gmail.com>
http://www.livejournal.com/users/jmmv/
The NetBSD Project - http://www.NetBSD.org/