* [PATCH] gssd: Error out when rpc_pipefs directory is empty @ 2014-07-08 14:33 Steve Dickson 2014-07-09 10:32 ` Jeff Layton 2014-07-09 19:11 ` Steve Dickson 0 siblings, 2 replies; 6+ messages in thread From: Steve Dickson @ 2014-07-08 14:33 UTC (permalink / raw) To: Linux NFS Mailing list 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty 2014-07-08 14:33 [PATCH] gssd: Error out when rpc_pipefs directory is empty Steve Dickson @ 2014-07-09 10:32 ` Jeff Layton 2014-07-09 18:21 ` Steve Dickson 2014-07-09 19:11 ` Steve Dickson 1 sibling, 1 reply; 6+ messages in thread From: Jeff Layton @ 2014-07-09 10:32 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing list 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty 2014-07-09 10:32 ` Jeff Layton @ 2014-07-09 18:21 ` Steve Dickson 2014-07-09 18:41 ` Jeff Layton 0 siblings, 1 reply; 6+ messages in thread From: Steve Dickson @ 2014-07-09 18:21 UTC (permalink / raw) To: Jeff Layton; +Cc: Linux NFS Mailing list 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty 2014-07-09 18:21 ` Steve Dickson @ 2014-07-09 18:41 ` Jeff Layton 2014-07-09 19:07 ` Steve Dickson 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2014-07-09 18:41 UTC (permalink / raw) To: Steve Dickson; +Cc: Jeff Layton, Linux NFS Mailing list 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty 2014-07-09 18:41 ` Jeff Layton @ 2014-07-09 19:07 ` Steve Dickson 0 siblings, 0 replies; 6+ messages in thread From: Steve Dickson @ 2014-07-09 19:07 UTC (permalink / raw) To: Jeff Layton; +Cc: Linux NFS Mailing list 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gssd: Error out when rpc_pipefs directory is empty 2014-07-08 14:33 [PATCH] gssd: Error out when rpc_pipefs directory is empty Steve Dickson 2014-07-09 10:32 ` Jeff Layton @ 2014-07-09 19:11 ` Steve Dickson 1 sibling, 0 replies; 6+ messages in thread From: Steve Dickson @ 2014-07-09 19:11 UTC (permalink / raw) To: Linux NFS Mailing list 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"); > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-09 19:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-08 14:33 [PATCH] gssd: Error out when rpc_pipefs directory is empty Steve Dickson 2014-07-09 10:32 ` Jeff Layton 2014-07-09 18:21 ` Steve Dickson 2014-07-09 18:41 ` Jeff Layton 2014-07-09 19:07 ` Steve Dickson 2014-07-09 19:11 ` Steve Dickson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox