[Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit

[Cluster-devel] [PATCH 0/2][RFC] pruning libcman a bit

[Jan Pokorný]

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

[Cluster-devel] [PATCH 1/2] libcman: E{INTR, AGAIN} in cman_dispatch repeated read needs -1 too

[Jan Pokorný]

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

[Cluster-devel] [PATCH 2/2] libcman: remove superfluous cman_dispatch outer loop condition

[Jan Pokorný]

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