* Minor change to alarm_handler to avoid code duplication
@ 2007-06-07 13:49 Anders Blomdell
2007-06-11 4:52 ` Ian Kent
2007-06-11 5:06 ` Ian Kent
0 siblings, 2 replies; 5+ messages in thread
From: Anders Blomdell @ 2007-06-07 13:49 UTC (permalink / raw)
To: autofs
[-- Attachment #1: Type: text/plain, Size: 147 bytes --]
As per request from discussions in
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241780
a patch against git.
Best regards
Anders Blomdell
[-- Attachment #2: alarm_handler.patch --]
[-- Type: text/plain, Size: 3421 bytes --]
--- autofs/lib/alarm.c 2007-06-07 15:05:03.000000000 +0200
+++ /usr/src/autofs/autofs.patched/lib/alarm.c 2007-06-07 15:08:29.000000000 +0200
@@ -28,16 +28,16 @@
#define alarm_lock() \
do { \
- int _alm_lock = pthread_mutex_lock(&mutex); \
- if (_alm_lock) \
- fatal(_alm_lock); \
+ int status = pthread_mutex_lock(&mutex); \
+ if (status) \
+ fatal(status); \
} while (0)
#define alarm_unlock() \
do { \
- int _alm_unlock = pthread_mutex_unlock(&mutex); \
- if (_alm_unlock) \
- fatal(_alm_unlock); \
+ int status = pthread_mutex_unlock(&mutex); \
+ if (status) \
+ fatal(status); \
} while (0)
void dump_alarms(void)
@@ -168,87 +168,65 @@
static void *alarm_handler(void *arg)
{
struct list_head *head;
- struct timespec expire;
- time_t now;
int status;
-
+
alarm_lock();
head = &alarms;
while (1) {
- struct alarm *current;
-
- /*
- * If the alarm list is empty, wait until an alarm is
- * added.
- */
- while (list_empty(head)) {
+ if (list_empty(head)) {
+ /* No alarms, wait for one to be added */
status = pthread_cond_wait(&cond, &mutex);
if (status)
fatal(status);
- }
-
- current = list_entry(head->next, struct alarm, list);
-
- now = time(NULL);
-
- if (current->time <= now) {
- struct autofs_point *ap;
-
- list_del(¤t->list);
-
- if (current->cancel) {
- free(current);
- continue;
- }
-
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- continue;
- }
-
- expire.tv_sec = current->time;
- expire.tv_nsec = 0;
-
- while (1) {
- struct autofs_point *ap;
- struct alarm *next;
-
- status = pthread_cond_timedwait(&cond, &mutex, &expire);
- if (status && status != ETIMEDOUT)
- fatal(status);
-
- next = list_entry(head->next, struct alarm, list);
- if (next->cancel) {
- list_del(&next->list);
- free(next);
- break;
+ } else {
+ struct alarm *first;
+ time_t now;
+
+ first = list_entry(head->next, struct alarm, list);
+
+ now = time(NULL);
+
+ if (first->time > now) {
+ /* Wait for alarm to trigger or a new alarm
+ to be added */
+ struct timespec expire;
+
+ expire.tv_sec = first->time;
+ expire.tv_nsec = 0;
+
+ status = pthread_cond_timedwait(&cond, &mutex,
+ &expire);
+ if (status && status != ETIMEDOUT)
+ fatal(status);
+ } else {
+ /* First alarm has triggered, run it */
+ struct autofs_point *ap;
+
+ list_del(&first->list);
+ ap = first->ap;
+ if (!first->cancel) {
+ /* We need to unlock the alarm list
+ in case some other thread holds
+ the state_mutex_lock(ap), and is
+ currently trying to do some
+ alarm_* function (i.e if we don't
+ unlock, we might deadlock) */
+ alarm_unlock();
+
+ state_mutex_lock(ap);
+ nextstate(ap->state_pipe[1],
+ ST_EXPIRE);
+ state_mutex_unlock(ap);
+
+ alarm_lock();
+ }
+ free(first);
}
-
- if (next != current)
- break;
-
- list_del(¤t->list);
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- break;
}
}
+ /* Will never come here, so alarm_unlock is not necessary */
}
int alarm_start_handler(void)
[-- Attachment #3: Type: text/plain, Size: 140 bytes --]
_______________________________________________
autofs mailing list
autofs@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/autofs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Minor change to alarm_handler to avoid code duplication
2007-06-07 13:49 Minor change to alarm_handler to avoid code duplication Anders Blomdell
@ 2007-06-11 4:52 ` Ian Kent
2007-06-11 5:06 ` Ian Kent
1 sibling, 0 replies; 5+ messages in thread
From: Ian Kent @ 2007-06-11 4:52 UTC (permalink / raw)
To: Anders Blomdell; +Cc: autofs
On Thu, 2007-06-07 at 15:49 +0200, Anders Blomdell wrote:
> As per request from discussions in
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241780
> a patch against git.
>
> Best regards
>
> Anders Blomdell
> plain text document attachment (alarm_handler.patch)
> --- autofs/lib/alarm.c 2007-06-07 15:05:03.000000000 +0200
> +++ /usr/src/autofs/autofs.patched/lib/alarm.c 2007-06-07 15:08:29.000000000 +0200
> @@ -28,16 +28,16 @@
>
> #define alarm_lock() \
> do { \
> - int _alm_lock = pthread_mutex_lock(&mutex); \
> - if (_alm_lock) \
> - fatal(_alm_lock); \
> + int status = pthread_mutex_lock(&mutex); \
> + if (status) \
> + fatal(status); \
> } while (0)
>
> #define alarm_unlock() \
> do { \
> - int _alm_unlock = pthread_mutex_unlock(&mutex); \
> - if (_alm_unlock) \
> - fatal(_alm_unlock); \
> + int status = pthread_mutex_unlock(&mutex); \
> + if (status) \
> + fatal(status); \
> } while (0)
This isn't related to what your proposing.
A separate patch would have been better for this so that we could
discuss it independently of the alarm_handler patch.
This patch hunk reverts the code to what I originally wrote.
It was changed due to complaints about compile warning about reused
symbol names and "the right thing to do for readability". Personally I
prefer it this way but I'm not going to change this again.
Ian
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Minor change to alarm_handler to avoid code duplication
2007-06-07 13:49 Minor change to alarm_handler to avoid code duplication Anders Blomdell
2007-06-11 4:52 ` Ian Kent
@ 2007-06-11 5:06 ` Ian Kent
2007-06-13 4:13 ` Ian Kent
1 sibling, 1 reply; 5+ messages in thread
From: Ian Kent @ 2007-06-11 5:06 UTC (permalink / raw)
To: Anders Blomdell; +Cc: autofs
On Thu, 2007-06-07 at 15:49 +0200, Anders Blomdell wrote:
> As per request from discussions in
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241780
> a patch against git.
Yes, on closer inspection this version does look ok.
Let me think about it for a little while in case I missed something.
Ian
>
> Best regards
>
> Anders Blomdell
> plain text document attachment (alarm_handler.patch)
> --- autofs/lib/alarm.c 2007-06-07 15:05:03.000000000 +0200
> +++ /usr/src/autofs/autofs.patched/lib/alarm.c 2007-06-07 15:08:29.000000000 +0200
> @@ -28,16 +28,16 @@
>
> #define alarm_lock() \
> do { \
> - int _alm_lock = pthread_mutex_lock(&mutex); \
> - if (_alm_lock) \
> - fatal(_alm_lock); \
> + int status = pthread_mutex_lock(&mutex); \
> + if (status) \
> + fatal(status); \
> } while (0)
>
> #define alarm_unlock() \
> do { \
> - int _alm_unlock = pthread_mutex_unlock(&mutex); \
> - if (_alm_unlock) \
> - fatal(_alm_unlock); \
> + int status = pthread_mutex_unlock(&mutex); \
> + if (status) \
> + fatal(status); \
> } while (0)
>
> void dump_alarms(void)
> @@ -168,87 +168,65 @@
> static void *alarm_handler(void *arg)
> {
> struct list_head *head;
> - struct timespec expire;
> - time_t now;
> int status;
> -
> +
> alarm_lock();
>
> head = &alarms;
>
> while (1) {
> - struct alarm *current;
> -
> - /*
> - * If the alarm list is empty, wait until an alarm is
> - * added.
> - */
> - while (list_empty(head)) {
> + if (list_empty(head)) {
> + /* No alarms, wait for one to be added */
> status = pthread_cond_wait(&cond, &mutex);
> if (status)
> fatal(status);
> - }
> -
> - current = list_entry(head->next, struct alarm, list);
> -
> - now = time(NULL);
> -
> - if (current->time <= now) {
> - struct autofs_point *ap;
> -
> - list_del(¤t->list);
> -
> - if (current->cancel) {
> - free(current);
> - continue;
> - }
> -
> - ap = current->ap;
> - free(current);
> - alarm_unlock();
> -
> - state_mutex_lock(ap);
> - nextstate(ap->state_pipe[1], ST_EXPIRE);
> - state_mutex_unlock(ap);
> -
> - alarm_lock();
> - continue;
> - }
> -
> - expire.tv_sec = current->time;
> - expire.tv_nsec = 0;
> -
> - while (1) {
> - struct autofs_point *ap;
> - struct alarm *next;
> -
> - status = pthread_cond_timedwait(&cond, &mutex, &expire);
> - if (status && status != ETIMEDOUT)
> - fatal(status);
> -
> - next = list_entry(head->next, struct alarm, list);
> - if (next->cancel) {
> - list_del(&next->list);
> - free(next);
> - break;
> + } else {
> + struct alarm *first;
> + time_t now;
> +
> + first = list_entry(head->next, struct alarm, list);
> +
> + now = time(NULL);
> +
> + if (first->time > now) {
> + /* Wait for alarm to trigger or a new alarm
> + to be added */
> + struct timespec expire;
> +
> + expire.tv_sec = first->time;
> + expire.tv_nsec = 0;
> +
> + status = pthread_cond_timedwait(&cond, &mutex,
> + &expire);
> + if (status && status != ETIMEDOUT)
> + fatal(status);
> + } else {
> + /* First alarm has triggered, run it */
> + struct autofs_point *ap;
> +
> + list_del(&first->list);
> + ap = first->ap;
> + if (!first->cancel) {
> + /* We need to unlock the alarm list
> + in case some other thread holds
> + the state_mutex_lock(ap), and is
> + currently trying to do some
> + alarm_* function (i.e if we don't
> + unlock, we might deadlock) */
> + alarm_unlock();
> +
> + state_mutex_lock(ap);
> + nextstate(ap->state_pipe[1],
> + ST_EXPIRE);
> + state_mutex_unlock(ap);
> +
> + alarm_lock();
> + }
> + free(first);
> }
> -
> - if (next != current)
> - break;
> -
> - list_del(¤t->list);
> - ap = current->ap;
> - free(current);
> - alarm_unlock();
> -
> - state_mutex_lock(ap);
> - nextstate(ap->state_pipe[1], ST_EXPIRE);
> - state_mutex_unlock(ap);
> -
> - alarm_lock();
> - break;
> }
> }
> + /* Will never come here, so alarm_unlock is not necessary */
> }
>
> int alarm_start_handler(void)
> _______________________________________________
> autofs mailing list
> autofs@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/autofs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Minor change to alarm_handler to avoid code duplication
2007-06-11 5:06 ` Ian Kent
@ 2007-06-13 4:13 ` Ian Kent
0 siblings, 0 replies; 5+ messages in thread
From: Ian Kent @ 2007-06-13 4:13 UTC (permalink / raw)
To: Anders Blomdell; +Cc: autofs
On Mon, 2007-06-11 at 13:06 +0800, Ian Kent wrote:
> On Thu, 2007-06-07 at 15:49 +0200, Anders Blomdell wrote:
> > As per request from discussions in
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241780
> > a patch against git.
>
> Yes, on closer inspection this version does look ok.
> Let me think about it for a little while in case I missed something.
>
Hi Anders,
This does indeed give some simplification to the alarm_handler function
and, as far as I can tell, retains the semantics of the original
function.
I've changed the patch around a little.
I removed the change to the mutex macros, in line with my comments
before, and I've re-structured the code a little.
Could you have a look and check that I haven't made any stupid mistakes
and that the patch still works the way you intended please.
Ian
---
diff --git a/lib/alarm.c b/lib/alarm.c
index 4a72605..c6c4ba3 100755
--- a/lib/alarm.c
+++ b/lib/alarm.c
@@ -169,6 +169,7 @@ static void *alarm_handler(void *arg)
{
struct list_head *head;
struct timespec expire;
+ struct alarm *first;
time_t now;
int status;
@@ -177,78 +178,56 @@ static void *alarm_handler(void *arg)
head = &alarms;
while (1) {
- struct alarm *current;
- /*
- * If the alarm list is empty, wait until an alarm is
- * added.
- */
- while (list_empty(head)) {
+ if (list_empty(head)) {
+ /* No alarms, wait for one to be added */
status = pthread_cond_wait(&cond, &mutex);
if (status)
fatal(status);
+ continue;
}
- current = list_entry(head->next, struct alarm, list);
+ first = list_entry(head->next, struct alarm, list);
now = time(NULL);
- if (current->time <= now) {
- struct autofs_point *ap;
-
- list_del(¤t->list);
-
- if (current->cancel) {
- free(current);
- continue;
- }
-
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- continue;
- }
-
- expire.tv_sec = current->time;
- expire.tv_nsec = 0;
-
- while (1) {
- struct autofs_point *ap;
- struct alarm *next;
+ if (first->time > now) {
+ /*
+ * Wait for alarm to trigger or a new alarm
+ * to be added.
+ */
+ expire.tv_sec = first->time;
+ expire.tv_nsec = 0;
status = pthread_cond_timedwait(&cond, &mutex, &expire);
if (status && status != ETIMEDOUT)
fatal(status);
-
- next = list_entry(head->next, struct alarm, list);
- if (next->cancel) {
- list_del(&next->list);
- free(next);
- break;
+ } else {
+ /* First alarm has triggered, run it */
+
+ list_del(&first->list);
+
+ if (!first->cancel) {
+ struct autofs_point *ap = first->ap;
+ /*
+ * We need to unlock the alarm list in case
+ * some other thread holds the state_mutex
+ *_lock(ap), and is currently trying to do
+ * some alarm_* function (i.e if we don't
+ * unlock, we might deadlock).
+ */
+ alarm_unlock();
+
+ state_mutex_lock(ap);
+ nextstate(ap->state_pipe[1], ST_EXPIRE);
+ state_mutex_unlock(ap);
+
+ alarm_lock();
}
-
- if (next != current)
- break;
-
- list_del(¤t->list);
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- break;
+ free(first);
}
}
+ /* Will never come here, so alarm_unlock is not necessary */
}
int alarm_start_handler(void)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Minor change to alarm_handler to avoid code duplication
@ 2007-06-08 7:19 Anders Blomdell
0 siblings, 0 replies; 5+ messages in thread
From: Anders Blomdell @ 2007-06-08 7:19 UTC (permalink / raw)
To: autofs
[-- Attachment #1: Type: text/plain, Size: 148 bytes --]
As per request from discussions in
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241780
a patch against git.
Best regards
Anders Blomdell
[-- Attachment #2: alarm_handler.patch --]
[-- Type: text/plain, Size: 3422 bytes --]
--- autofs/lib/alarm.c 2007-06-07 15:05:03.000000000 +0200
+++ /usr/src/autofs/autofs.patched/lib/alarm.c 2007-06-07 15:08:29.000000000 +0200
@@ -28,16 +28,16 @@
#define alarm_lock() \
do { \
- int _alm_lock = pthread_mutex_lock(&mutex); \
- if (_alm_lock) \
- fatal(_alm_lock); \
+ int status = pthread_mutex_lock(&mutex); \
+ if (status) \
+ fatal(status); \
} while (0)
#define alarm_unlock() \
do { \
- int _alm_unlock = pthread_mutex_unlock(&mutex); \
- if (_alm_unlock) \
- fatal(_alm_unlock); \
+ int status = pthread_mutex_unlock(&mutex); \
+ if (status) \
+ fatal(status); \
} while (0)
void dump_alarms(void)
@@ -168,87 +168,65 @@
static void *alarm_handler(void *arg)
{
struct list_head *head;
- struct timespec expire;
- time_t now;
int status;
-
+
alarm_lock();
head = &alarms;
while (1) {
- struct alarm *current;
-
- /*
- * If the alarm list is empty, wait until an alarm is
- * added.
- */
- while (list_empty(head)) {
+ if (list_empty(head)) {
+ /* No alarms, wait for one to be added */
status = pthread_cond_wait(&cond, &mutex);
if (status)
fatal(status);
- }
-
- current = list_entry(head->next, struct alarm, list);
-
- now = time(NULL);
-
- if (current->time <= now) {
- struct autofs_point *ap;
-
- list_del(¤t->list);
-
- if (current->cancel) {
- free(current);
- continue;
- }
-
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- continue;
- }
-
- expire.tv_sec = current->time;
- expire.tv_nsec = 0;
-
- while (1) {
- struct autofs_point *ap;
- struct alarm *next;
-
- status = pthread_cond_timedwait(&cond, &mutex, &expire);
- if (status && status != ETIMEDOUT)
- fatal(status);
-
- next = list_entry(head->next, struct alarm, list);
- if (next->cancel) {
- list_del(&next->list);
- free(next);
- break;
+ } else {
+ struct alarm *first;
+ time_t now;
+
+ first = list_entry(head->next, struct alarm, list);
+
+ now = time(NULL);
+
+ if (first->time > now) {
+ /* Wait for alarm to trigger or a new alarm
+ to be added */
+ struct timespec expire;
+
+ expire.tv_sec = first->time;
+ expire.tv_nsec = 0;
+
+ status = pthread_cond_timedwait(&cond, &mutex,
+ &expire);
+ if (status && status != ETIMEDOUT)
+ fatal(status);
+ } else {
+ /* First alarm has triggered, run it */
+ struct autofs_point *ap;
+
+ list_del(&first->list);
+ ap = first->ap;
+ if (!first->cancel) {
+ /* We need to unlock the alarm list
+ in case some other thread holds
+ the state_mutex_lock(ap), and is
+ currently trying to do some
+ alarm_* function (i.e if we don't
+ unlock, we might deadlock) */
+ alarm_unlock();
+
+ state_mutex_lock(ap);
+ nextstate(ap->state_pipe[1],
+ ST_EXPIRE);
+ state_mutex_unlock(ap);
+
+ alarm_lock();
+ }
+ free(first);
}
-
- if (next != current)
- break;
-
- list_del(¤t->list);
- ap = current->ap;
- free(current);
- alarm_unlock();
-
- state_mutex_lock(ap);
- nextstate(ap->state_pipe[1], ST_EXPIRE);
- state_mutex_unlock(ap);
-
- alarm_lock();
- break;
}
}
+ /* Will never come here, so alarm_unlock is not necessary */
}
int alarm_start_handler(void)
[-- Attachment #3: Type: text/plain, Size: 140 bytes --]
_______________________________________________
autofs mailing list
autofs@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/autofs
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-06-13 4:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-06-07 13:49 Minor change to alarm_handler to avoid code duplication Anders Blomdell
2007-06-11 4:52 ` Ian Kent
2007-06-11 5:06 ` Ian Kent
2007-06-13 4:13 ` Ian Kent
-- strict thread matches above, loose matches on Subject: below --
2007-06-08 7:19 Anders Blomdell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.