* [Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit
@ 2012-10-09 18:31 Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 1/2] libcman: E{INTR, AGAIN} in cman_dispatch repeated read needs -1 too Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 2/2] libcman: remove superfluous cman_dispatch outer loop condition Jan Pokorný
0 siblings, 2 replies; 3+ messages in thread
From: Jan Pokorný @ 2012-10-09 18:31 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hello,
I discovered two questionable places in libcman fixing of which has
a tiny simplification of code as a side-effect.
I tried my best to validate the changes (esp. the second changeset),
but would be great if someone reviewed the drafted thought-process
behind the changes as well as the actual changes.
Jan Pokorn? (2):
libcman: E{INTR,AGAIN} in cman_dispatch repeated read needs -1 too
libcman: remove superfluous cman_dispatch outer loop condition
cman/lib/libcman.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
--
1.7.11.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH 1/2] libcman: E{INTR, AGAIN} in cman_dispatch repeated read needs -1 too
2012-10-09 18:31 [Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit Jan Pokorný
@ 2012-10-09 18:31 ` Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 2/2] libcman: remove superfluous cman_dispatch outer loop condition Jan Pokorný
1 sibling, 0 replies; 3+ messages in thread
From: Jan Pokorný @ 2012-10-09 18:31 UTC (permalink / raw)
To: cluster-devel.redhat.com
Because otherwise, all you get during next cman_dispatch is noise
(i.e., the incoming messages are misaligned wrt. their boundaries).
In better case, this is captured as erroneous field value immediately,
otherwise the result will be pretty crazy.
The hypothetical "proper" fix is either:
- making sure EINTR/EAGAIN does not break atomicity of message
receiving (which in turn may break nonblocking character)
- the part of yet-read message is stored in the handle
(similar to reply_buf{fer,len} items)
Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
cman/lib/libcman.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 6ed8ecb..26c09f2 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -539,13 +539,6 @@ int cman_dispatch(cman_handle_t handle, int flags)
return -1;
}
- if (len < 0 &&
- (errno == EINTR || errno == EAGAIN)) {
- if (bufptr != buf)
- free(bufptr);
- return 0;
- }
-
if (len < 0) {
if (bufptr != buf)
free(bufptr);
--
1.7.11.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH 2/2] libcman: remove superfluous cman_dispatch outer loop condition
2012-10-09 18:31 [Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 1/2] libcman: E{INTR, AGAIN} in cman_dispatch repeated read needs -1 too Jan Pokorný
@ 2012-10-09 18:31 ` Jan Pokorný
1 sibling, 0 replies; 3+ messages in thread
From: Jan Pokorný @ 2012-10-09 18:31 UTC (permalink / raw)
To: cluster-devel.redhat.com
My grasp is that "len < 0" will never occur@the point of condition
check [*]. Under this assumption, the other check (for errno != EAGAIN)
does not decide anything, only making the code more obfuscated.
Please note that there used to be some discrepancies around in the past,
notably see commit bd910f7e9de2d320f10a12cd9a3e7a26fb00083a.
[*] proof is left ... ok, below:
We consider CMAN_DISPATCH_ALL only as it is prerequisite to even get
further in the evaluation of the discussed condition.
1. we can get into the loop condition when processing a message from
the waitlist and "len" is then set to the lenght of such handled
message
1a. message is added to the waitlist only indirectly from within
cman_dispatch, but cman_dispatch may proceed a received
message this far only if it was indeed received OK
(this includes nonzero lenght indicated in the header part
[although this is not checked directly], which is exactly
the value set to "len" checked in the discussed condition)
1b. hence, "len" cannot be negative when we reach the condition
this way (effectively skipping final errno comparison)
2. we can get into the loop condition also after processing directly
received message, where "len" carries the return value of either
recv or read
2a. any case of negative-or-zero "len" is captured immediately
after setting it, always resulting in returning from call
2b. ditto 1b.
3. 1b. + 2b. -> Q.E.D.
Please correct me if I am wrong.
Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
cman/lib/libcman.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 26c09f2..4ecdd35 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -554,8 +554,7 @@ int cman_dispatch(cman_handle_t handle, int flags)
if (res)
break;
- } while ( flags & CMAN_DISPATCH_ALL &&
- !(len < 0 && errno == EAGAIN) );
+ } while (flags & CMAN_DISPATCH_ALL);
return len;
}
--
1.7.11.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-10-09 18:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-09 18:31 [Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 1/2] libcman: E{INTR, AGAIN} in cman_dispatch repeated read needs -1 too Jan Pokorný
2012-10-09 18:31 ` [Cluster-devel] [PATCH 2/2] libcman: remove superfluous cman_dispatch outer loop condition Jan Pokorný
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.