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

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

[Steve Dickson]

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");
-- 
1.9.3

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

[Jeff Layton]

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.

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?

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.

-- 
Jeff Layton <jlayton@primarydata.com>

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

[Steve Dickson]

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!! ;-) 

steved.

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

[Jeff Layton]

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.


-- 
Jeff Layton <jlayton@primarydata.com>

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

[Steve Dickson]

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.

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

[Steve Dickson]

On 07/08/2014 10:33 AM, 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 <steved@redhat.com>
Committed... 

steved.

> ---
>  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");
>