From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:7103 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756558AbaGITHl (ORCPT ); Wed, 9 Jul 2014 15:07:41 -0400 Message-ID: <53BD92F4.4010906@RedHat.com> Date: Wed, 09 Jul 2014 15:07:32 -0400 From: Steve Dickson MIME-Version: 1.0 To: Jeff Layton CC: Linux NFS Mailing list Subject: Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty References: <1404830019-4299-1-git-send-email-steved@redhat.com> <20140709063200.6add2dce@tlielax.poochiereds.net> <53BD8836.9080806@RedHat.com> <20140709144122.4729279e@tlielax.poochiereds.net> In-Reply-To: <20140709144122.4729279e@tlielax.poochiereds.net> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: On 07/09/2014 02:41 PM, Jeff Layton wrote: > On Wed, 09 Jul 2014 14:21:42 -0400 > Steve Dickson wrote: > >> Hey Jeff, >> >> On 07/09/2014 06:32 AM, Jeff Layton wrote: >>> On Tue, 8 Jul 2014 10:33:39 -0400 >>> Steve Dickson wrote: >>> >>>> When there is no kernel modules loaded the rpc_pipefs >>>> directory is empty, which cause rpc.gssd to silently >>>> exit. >>>> >>>> This patch adds a check to see if the topdirs_list >>>> is empty. If so error out without dropping a core. >>>> >>>> Signed-off-by: Steve Dickson >>>> --- >>>> utils/gssd/gssd_main_loop.c | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c >>>> index 9970028..6946ab6 100644 >>>> --- a/utils/gssd/gssd_main_loop.c >>>> +++ b/utils/gssd/gssd_main_loop.c >>>> @@ -173,6 +173,10 @@ topdirs_init_list(void) >>>> if (ret) >>>> goto out_err; >>>> } >>>> + if (TAILQ_EMPTY(&topdirs_list)) { >>>> + printerr(0, "ERROR: rpc_pipefs directory '%s' is empty!\n", pipefs_dir); >>>> + return -1; >>>> + } >>>> closedir(pipedir); >>>> return 0; >>>> out_err: >>>> @@ -233,9 +237,10 @@ gssd_run() >>>> sigaddset(&set, DNOTIFY_SIGNAL); >>>> sigprocmask(SIG_UNBLOCK, &set, NULL); >>>> >>>> - if (topdirs_init_list() != 0) >>>> - return; >>>> - >>>> + if (topdirs_init_list() != 0) { >>>> + /* Error msg is already printed */ >>>> + exit(1); >>>> + } >>>> init_client_list(); >>>> >>>> printerr(1, "beginning poll\n"); >>> >>> Does it drop a core now? It looks sort of like it would just exit(1) >>> silently. >> No. But whenever gssd_run() returns, in main, abort() is called, >> which is probably a bit brain dead... but it is what it is... >> >>> >>> In any case, this patch is certainly better than crashing, but gssd >>> looks sort of like it's doing the wrong thing here. Should it not just >>> wait idly for directories to show up instead of exiting if none are >>> present? >> The purpose/cope of this patch is to stop gssd from silently exiting >> which the patch does... >> >> The question of whether gssd should or should not be exiting is >> a different problem... a problem this patch is not trying to solve. >> >> I tend to agree with you, that gssd probably should just hang out >> but in what scenario is gssd start and none of the kernel modules >> are loaded... Its not a normal one... >> >>> >>> Also, because topdir_init_list is run only once, it looks like gssd >>> doesn't properly handle the case where there may be some directories in >>> rpc_pipefs when gssd starts, but then others show up later. Any that >>> show up after gssd is started are just ignored currently, right? That >>> seems like a subtle source of bugs if you just happen to start gssd a >>> little early. >>> >> Again all this patch does is document the exit... Not processing late >> showing up directories would be a problem.... but as always.... >> patches are welcomed!! ;-) >> > > Fair enough. I don't forsee myself having much time for this anytime > soon, so the best we can do is probably make note of it until someone > else has the time and desire to fix it. > > It's always hard to predict the order that things will be started, > particularly now that we have a trend toward more parallel startup with > systemd. Having daemons that are robust enough to deal with it when > things are not well-ordered is certainly ideal. I can't agree with you more!!! steved.