On Wed, 9 Jan 2008 17:45:06 +0000
Christoph Hellwig <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 02:33:17PM -0500, Jeff Layton wrote:
> > - struct svc_serv * serv;
> > - int error = 0;
> > + struct svc_serv *serv;
> > + struct svc_rqst *rqstp;
> > + int error = 0;
> >
> > mutex_lock(&nlmsvc_mutex);
> > /*
> > * Check whether we're already up and running.
> > */
> > - if (nlmsvc_pid) {
> > + if (nlmsvc_task) {
> > if (proto)
> > error = make_socks(nlmsvc_serv, proto);
>
> While equivalent I think it would be clener to check for nlmsvc_serv
> above as that'swhat we're passing to make_socks. But I think the
> whole of lockd_up could use a little makeover, but that's for later.
>
Probably so. If I respin, I'll plan to fix that too.
> > void
> > lockd_down(void)
> > {
> > mutex_lock(&nlmsvc_mutex);
> > if (nlmsvc_users) {
> > if (--nlmsvc_users)
> > goto out;
> > + } else {
> > + printk(KERN_ERR "lockd_down: no users! task=%p\n",
> > + nlmsvc_task);
> > + BUG();
> > }
> > + if (!nlmsvc_task) {
> > + printk(KERN_ERR "lockd_down: no lockd running.\n");
> > + BUG();
> > }
> > + kthread_stop(nlmsvc_task);
>
> I think all this user/foo checking here should be BUG_ONs as it's
> quite fatal errors.
>
> e.g.
>
> void
> lockd_down(void)
> {
> mutex_lock(&nlmsvc_mutex);
>
> BUG_ON(!nlmsvc_task);
> BUG_ON(!nlmsvc_users);
>
> if (!--nlmsvc_users)
> kthread_stop(nlmsvc_task);
> mutex_unlock(&nlmsvc_mutex);
> }
>
>
> same applies for similar checks in lockd_up aswell.
>
With this patch the lockd_down checks should now be BUGs. I decided
not to do that in lockd_up. If there's an error within the main
lockd loop, it can exit without being requested to do so. If someone
then calls lockd_up then the counts will be off and the check will fire.
It seems like if we're going to make the check in lockd_up be a BUG,
then we should also BUG rather than letting lockd exit prematurely.
--
Jeff Layton <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
