Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty

[Steve Dickson <SteveD@redhat.com>]

On 07/09/2014 02:41 PM, Jeff Layton wrote:
> On Wed, 09 Jul 2014 14:21:42 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Hey Jeff,
>>
>> On 07/09/2014 06:32 AM, Jeff Layton wrote:
>>> On Tue,  8 Jul 2014 10:33:39 -0400
>>> Steve Dickson <steved@redhat.com> 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 <steved@redhat.com>
>>>> ---
>>>>  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.