[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: RFS: dhcp-probe, another try to request with a lot of update



[...]
> 
> OK, OK, i think that is not the cleaner method to run a rm -rf on a file
> which doesn't exist but if you want, and if it is the usage in debian
> project, i will do
> 

This is definitely a minor issue, but I'm just trying to get you to follow at
the very least the KISS principle (keep it simple and stupid) and some
efficiency, getting rid of unnecessary bloat.

[...]

> > - Your init.d file feels fairly hacked, please take a look at, e.g., the init.d
> >   file of the arpwatch package (which also runs multiple daemons for multiple
> >   interfaces). Just looking at the code you use to kill daemons makes me shiver.
> >   start-stop-daemon will do. Really. Please don't keep all the stuff you
> >   commented out, either remove it or implement it. force-reload, however, must
> >   get implemented (see Debian Policy Sec. 9.3.2).
> 
> your words "fairly hacked" is a joke ?
> 
No. Again, this init script does not follow the KISS principle and it does not
make use of LSB helper function, which makes it just all too complex. An init
script is an utterly simple thing: It should start and stop a daemon, and maybe
report whether it is running. As a rule of thumb, 8% of code contains bugs. The
large such a script is, the more bugs it will contain. And reviewing so much
code is more work as well. 

> The main problem i found in my init.d file was the signal used to kill
> the daemon (9) instead of the normal (15).
> For each configuration files, the script will read information about
> interface and its properties. Then script read daemon PID associated to
> configuration file and kill it and delete the PID file (added in latest
> update).

Why can't you just use start-stop-daemon? It will handle all that stuff.

> The force_stop function is here to handle, with a basic system view,
> cases where daemons have to be stop.
> Moreover, arpwatch package does not provide the status option if
> 'running*' function are sources of the reaction...
> 
> Could you explain me your comment ?
> 
My comment is mostly about the way you stop the daemons. There should not be a
need to attempt to kill it in so many ways, just let start-stop-daemon do the
job. If it fails -- so be it. Then something horribly bad is going on. But I
need to review your updated package before further commenting on this.

[...]

> 
> >   * lines 59,61: Only depend on install, you can't do build and install in
> >     parallel and install already depends on build
> >   * lines 62,63: dh_testdir is ok, no additional files required
> >   * line 69: I think defaults should be fine, why do you add 80 15; that
> >     contradicts "defaults"
> 
> According to tour advice i put "defaults" level to start daemon but i
> think that it is better to start the daemon before network start and
> stop it after.
> 

Isn't that the wrong way round? What is dhcp-probe good for, if there is no
network running? But anyway, I'm fine with custom numbers, but then don't put
defaults in there as well.

[...]

I'll grab the package from mentors.d.n later on this weekend and report back.

Best,
Michael


Attachment: pgpRJfdN1bbSm.pgp
Description: PGP signature


Reply to: