diff mbox

[4/4] rpc.gssd: don't call poll() twice a second.

Message ID 1343768449-32205-4-git-send-email-bfields@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bruce Fields July 31, 2012, 9 p.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before
2.4).

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/gssd/gssd_main_loop.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Chuck Lever July 31, 2012, 9:23 p.m. UTC | #1 
On Jul 31, 2012, at 2:00 PM, J. Bruce Fields wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Use ppoll instead.
> 
> (This breaks compatibility with kernels before 2.6.16 and glibc before
> 2.4).

I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> utils/gssd/gssd_main_loop.c |   12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
> index 142c8c5..2d20fa2 100644
> --- a/utils/gssd/gssd_main_loop.c
> +++ b/utils/gssd/gssd_main_loop.c
> @@ -186,13 +186,14 @@ gssd_run()
> 	struct sigaction	dn_act = {
> 		.sa_handler = dir_notify_handler
> 	};
> -	sigset_t		set;
> +	sigset_t		set, emptyset;
> 
> 	sigemptyset(&dn_act.sa_mask);
> 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
> 
> -	/* just in case the signal is blocked... */
> +	sigemptyset(&emptyset);
> 	sigemptyset(&set);
> +	/* just in case the signal is blocked... */
> 	sigaddset(&set, DNOTIFY_SIGNAL);
> 	sigprocmask(SIG_UNBLOCK, &set, NULL);
> 
> @@ -210,16 +211,13 @@ gssd_run()
> 				exit(1);
> 			}
> 		}
> -		/* race condition here: dir_changed could be set before we
> -		 * enter the poll, and we'd never notice if it weren't for the
> -		 * timeout. */
> -		ret = poll(pollarray, pollsize, POLL_MILLISECS);
> +		ret = ppoll(pollarray, pollsize, NULL, &emptyset);
> 		if (ret < 0) {
> 			if (errno != EINTR)
> 				printerr(0,
> 					 "WARNING: error return from poll\n");
> 		} else if (ret == 0) {
> -			/* timeout */
> +			printerr(0, "WARNING: unexpected timeout\n");
> 		} else { /* ret > 0 */
> 			scan_poll_results(ret);
> 		}
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bruce Fields July 31, 2012, 9:27 p.m. UTC | #2 
On Tue, Jul 31, 2012 at 02:23:36PM -0700, Chuck Lever wrote:
> 
> On Jul 31, 2012, at 2:00 PM, J. Bruce Fields wrote:
> 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > Use ppoll instead.
> > 
> > (This breaks compatibility with kernels before 2.6.16 and glibc before
> > 2.4).
> 
> I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

I was assuming we could drop support for older systems.

If not, then'd I'd much prefer to go back to the self-pipe trick than to
deal with autoconf and  ifdef'ing out all this stuff.

--b.

> 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> > utils/gssd/gssd_main_loop.c |   12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
> > index 142c8c5..2d20fa2 100644
> > --- a/utils/gssd/gssd_main_loop.c
> > +++ b/utils/gssd/gssd_main_loop.c
> > @@ -186,13 +186,14 @@ gssd_run()
> > 	struct sigaction	dn_act = {
> > 		.sa_handler = dir_notify_handler
> > 	};
> > -	sigset_t		set;
> > +	sigset_t		set, emptyset;
> > 
> > 	sigemptyset(&dn_act.sa_mask);
> > 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
> > 
> > -	/* just in case the signal is blocked... */
> > +	sigemptyset(&emptyset);
> > 	sigemptyset(&set);
> > +	/* just in case the signal is blocked... */
> > 	sigaddset(&set, DNOTIFY_SIGNAL);
> > 	sigprocmask(SIG_UNBLOCK, &set, NULL);
> > 
> > @@ -210,16 +211,13 @@ gssd_run()
> > 				exit(1);
> > 			}
> > 		}
> > -		/* race condition here: dir_changed could be set before we
> > -		 * enter the poll, and we'd never notice if it weren't for the
> > -		 * timeout. */
> > -		ret = poll(pollarray, pollsize, POLL_MILLISECS);
> > +		ret = ppoll(pollarray, pollsize, NULL, &emptyset);
> > 		if (ret < 0) {
> > 			if (errno != EINTR)
> > 				printerr(0,
> > 					 "WARNING: error return from poll\n");
> > 		} else if (ret == 0) {
> > -			/* timeout */
> > +			printerr(0, "WARNING: unexpected timeout\n");
> > 		} else { /* ret > 0 */
> > 			scan_poll_results(ret);
> > 		}
> > -- 
> > 1.7.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chuck Lever July 31, 2012, 10:29 p.m. UTC | #3 
On Jul 31, 2012, at 2:27 PM, J. Bruce Fields wrote:

> On Tue, Jul 31, 2012 at 02:23:36PM -0700, Chuck Lever wrote:
>> 
>> On Jul 31, 2012, at 2:00 PM, J. Bruce Fields wrote:
>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> Use ppoll instead.
>>> 
>>> (This breaks compatibility with kernels before 2.6.16 and glibc before
>>> 2.4).
>> 
>> I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.
> 
> I was assuming we could drop support for older systems.

We went to a lot of trouble to get the IPv6 stuff to build correctly on older systems, and we still heard complaints.

> If not, then'd I'd much prefer to go back to the self-pipe trick than to
> deal with autoconf and  ifdef'ing out all this stuff.

This patch is a tiny change.  I don't think autoconf in this case is onerous.  I'm happy to help you with it.

> --b.
> 
>> 
>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>> ---
>>> utils/gssd/gssd_main_loop.c |   12 +++++-------
>>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
>>> index 142c8c5..2d20fa2 100644
>>> --- a/utils/gssd/gssd_main_loop.c
>>> +++ b/utils/gssd/gssd_main_loop.c
>>> @@ -186,13 +186,14 @@ gssd_run()
>>> 	struct sigaction	dn_act = {
>>> 		.sa_handler = dir_notify_handler
>>> 	};
>>> -	sigset_t		set;
>>> +	sigset_t		set, emptyset;
>>> 
>>> 	sigemptyset(&dn_act.sa_mask);
>>> 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
>>> 
>>> -	/* just in case the signal is blocked... */
>>> +	sigemptyset(&emptyset);
>>> 	sigemptyset(&set);
>>> +	/* just in case the signal is blocked... */
>>> 	sigaddset(&set, DNOTIFY_SIGNAL);
>>> 	sigprocmask(SIG_UNBLOCK, &set, NULL);
>>> 
>>> @@ -210,16 +211,13 @@ gssd_run()
>>> 				exit(1);
>>> 			}
>>> 		}
>>> -		/* race condition here: dir_changed could be set before we
>>> -		 * enter the poll, and we'd never notice if it weren't for the
>>> -		 * timeout. */
>>> -		ret = poll(pollarray, pollsize, POLL_MILLISECS);
>>> +		ret = ppoll(pollarray, pollsize, NULL, &emptyset);
>>> 		if (ret < 0) {
>>> 			if (errno != EINTR)
>>> 				printerr(0,
>>> 					 "WARNING: error return from poll\n");
>>> 		} else if (ret == 0) {
>>> -			/* timeout */
>>> +			printerr(0, "WARNING: unexpected timeout\n");
>>> 		} else { /* ret > 0 */
>>> 			scan_poll_results(ret);
>>> 		}
>>> -- 
>>> 1.7.9.5
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Dickson Aug. 1, 2012, 12:35 p.m. UTC | #4 
On 07/31/2012 06:29 PM, Chuck Lever wrote:
> 
> On Jul 31, 2012, at 2:27 PM, J. Bruce Fields wrote:
> 
>> On Tue, Jul 31, 2012 at 02:23:36PM -0700, Chuck Lever wrote:
>>>
>>> On Jul 31, 2012, at 2:00 PM, J. Bruce Fields wrote:
>>>
>>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>>>
>>>> Use ppoll instead.
>>>>
>>>> (This breaks compatibility with kernels before 2.6.16 and glibc before
>>>> 2.4).
>>>
>>> I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.
>>
>> I was assuming we could drop support for older systems.
> 
> We went to a lot of trouble to get the IPv6 stuff to build correctly on older systems, and we still heard complaints.
> 
>> If not, then'd I'd much prefer to go back to the self-pipe trick than to
>> deal with autoconf and  ifdef'ing out all this stuff.
> 
> This patch is a tiny change.  I don't think autoconf in this case is onerous.  I'm happy to help you with it.
I think it would be work the effort... Let me know if I can help...

steved.
 
> 
>> --b.
>>
>>>
>>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>>> ---
>>>> utils/gssd/gssd_main_loop.c |   12 +++++-------
>>>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
>>>> index 142c8c5..2d20fa2 100644
>>>> --- a/utils/gssd/gssd_main_loop.c
>>>> +++ b/utils/gssd/gssd_main_loop.c
>>>> @@ -186,13 +186,14 @@ gssd_run()
>>>> 	struct sigaction	dn_act = {
>>>> 		.sa_handler = dir_notify_handler
>>>> 	};
>>>> -	sigset_t		set;
>>>> +	sigset_t		set, emptyset;
>>>>
>>>> 	sigemptyset(&dn_act.sa_mask);
>>>> 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
>>>>
>>>> -	/* just in case the signal is blocked... */
>>>> +	sigemptyset(&emptyset);
>>>> 	sigemptyset(&set);
>>>> +	/* just in case the signal is blocked... */
>>>> 	sigaddset(&set, DNOTIFY_SIGNAL);
>>>> 	sigprocmask(SIG_UNBLOCK, &set, NULL);
>>>>
>>>> @@ -210,16 +211,13 @@ gssd_run()
>>>> 				exit(1);
>>>> 			}
>>>> 		}
>>>> -		/* race condition here: dir_changed could be set before we
>>>> -		 * enter the poll, and we'd never notice if it weren't for the
>>>> -		 * timeout. */
>>>> -		ret = poll(pollarray, pollsize, POLL_MILLISECS);
>>>> +		ret = ppoll(pollarray, pollsize, NULL, &emptyset);
>>>> 		if (ret < 0) {
>>>> 			if (errno != EINTR)
>>>> 				printerr(0,
>>>> 					 "WARNING: error return from poll\n");
>>>> 		} else if (ret == 0) {
>>>> -			/* timeout */
>>>> +			printerr(0, "WARNING: unexpected timeout\n");
>>>> 		} else { /* ret > 0 */
>>>> 			scan_poll_results(ret);
>>>> 		}
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Chuck Lever
>>> chuck[dot]lever[at]oracle[dot]com
>>>
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
index 142c8c5..2d20fa2 100644
--- a/utils/gssd/gssd_main_loop.c
+++ b/utils/gssd/gssd_main_loop.c
@@ -186,13 +186,14 @@  gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 
@@ -210,16 +211,13 @@  gssd_run()
 				exit(1);
 			}
 		}
-		/* race condition here: dir_changed could be set before we
-		 * enter the poll, and we'd never notice if it weren't for the
-		 * timeout. */
-		ret = poll(pollarray, pollsize, POLL_MILLISECS);
+		ret = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}