Re: RFS: fsprotect
Thanks for the review Matthew. Look inline.
On Sunday 22 March 2009, Matthew Palmer wrote:
> On Sun, Mar 22, 2009 at 07:21:22PM +0200, Stefanos Harhalakis wrote:
> > * Package name : fsprotect
> > Version : 1.0.0
> > Upstream Author : Stefanos Harhalakis <v13@v13.gr> (me)
> > * URL : http://www.v13.gr/ (not available yet)
>
> Based on your e-mail address, I'm guessing that www.v13.gr isn't going to
> be dedicated to fsprotect, so you should adjust this URL to point to the
> fsprotect project.
I'll add it when it is available. Since this is a debian native package, I'm
waiting for it to enter debian (if it ever happens) before creating a page.
> > It builds these binary packages:
> > fsprotect - Helper scripts to make the filesystems immutable
>
> s/the//. Also, I'm not sure "immutable" is quite the right word here; I'm
> having trouble coming up with a better short description, though.
Fixed the "the".
> > The package appears to be lintian clean.
>
> Oh. No. It. Ain't.
Unless you're talking about the debconf related errors/notes and/or the
non-standard-toplevel-dir, please give me a more specific hint.
> > This package ignores some lintian errors/warnings. I believe that all of
> > them are justified:
> >
> > non-standard-toplevel-dir fsprotect/
> >
> > : This is required for fsprotect to work. This directory must exist in
> > : the
> >
> > root filesystem.
>
> *A* directory must exist in the root filesystem. I don't think a new
> top-level directory is justified for this. /lib would appear to be the
> usual location for things of this nature.
FWIW: A directory at the root filesystem is required at the very early stages
of boot (while running scripts in initramfs) just after the root filesystem
is mounted.
You suggest that I move this from / to /lib/init (/lib/init/fsprotect) ?
This may be somehow ugly when viewing mountpoints since there will exist
mounts similar to: /lib/init/fsprotect/fs/home/orig
Keep in mind that fsprotect is not an addon package. It changes the boot
procedure and has some special needs. Having a specific top-level directory
for that may make things more readable in the future.
Anyway, if /fsprotect isn't acceptable and /lib/init/fsprotect is, I can
change it. Just confirm it.
> > fsprotect: no-debconf-config
> >
> > : There is no need for debconf config file. It only needs to display a
> > : warning
> >
> > message/note. If this is not OK I'll remove the note completely.
>
> That note is unnecessary. README.Debian is the correct place for
> information like this, which you've already provided.
Removed the message and all debconf related things.
> > fsprotect: virtual-package-depends-without-real-package-depends
> >
> > : This package is never going to be a build dependency for other
> > : packages, so
> >
> > this should be OK.
>
> Never say never... on the other hand, keeping your depends list up to date
> with new kernel module versions isn't likely to be a whole lot of fun.
Exactly. Let me postpone that change until a package build-depends on it :-)
> > init.d-script-does-not-implement-required-option /etc/init.d/fsprotect
> > force-reload
> >
> > : It is imposible to provide a force-reload or restart option. Even the
> > : stop
> >
> > is a no-op.
> >
> > init.d-script-does-not-implement-required-option /etc/init.d/fsprotect
> > restart
> >
> > : Same as above
>
> If the stop option is a no-op along with force-reload and restart, why did
> you implement stop and not the others? Take a look at mountall.sh.
mountall.sh is what I looked at when creating it. I did the modification you
suggested but I'm not sure that:
echo "Error: argument '$1' not supported" >&2 ; exit 3
adds anything usefull to:
echo "Usage: $SCRIPTNAME {start|stop}" >&2 ; exit 3
which was ran when restart/reload/force-reload was used. As a see it, the
mountall approach is superfluous (no?).
> > p.s. Please CC me. I'm not subscribed to the list.
>
> Mail-Followup-To is your friend.
It's a big pain to add this to mails for this list only (I'm using kmail). An
older thread in debian-mentors suggested that adding this note is a right
thing (tm) to do.
I've uploaded v1.0.1 to mentors with those fixes except from the /fsprotect
directory.
Reply to: