ceph: messenger: read_partial() cleanups

PATCH In review Alex Elder 2 participants 9 messages May 10, 2012 – May 14, 2012 Ceph common code (libceph)

Share & tools

Alex Elder

author

2012-05-10 22:56 UTC #0 lore raw

cover ceph: messenger: read_partial() cleanups

This short series adds the use of read_partial() in a few places that it is not already. It also gets rid of the in/out "to" argument (which continues to cause confusion every time I see it), using an in-only "end" argument in its place.

-Alex

Alex Elder

author

2012-05-11 01:18 UTC #1 lore raw

PATCH 1/3 ceph: messenger: use read_partial() in read_partial_message()

There are two blocks of code in read_partial_message()--those that read the header and footer of the message--that can be replaced by a call to read_partial(). Do that.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

net/ceph/messenger.c +10 ~~-20~~

index f0993af..673133e 100644
@@ -1628,7 +1628,7 @@ static int read_partial_message(struct ceph_connection *con)
 {
 	struct ceph_msg *m = con->in_msg;
 	int ret;
-	int to, left;
+	int to;
 	unsigned front_len, middle_len, data_len;
 	bool do_datacrc = !con->msgr->nocrc;
 	int skip;

@@ -1638,15 +1638,10 @@ static int read_partial_message(struct ceph_connection *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	while (con->in_base_pos < sizeof(con->in_hdr)) {
-		left = sizeof(con->in_hdr) - con->in_base_pos;
-		ret = ceph_tcp_recvmsg(con->sock,
-				       (char *)&con->in_hdr + con->in_base_pos,
-				       left);
-		if (ret <= 0)
-			return ret;
-		con->in_base_pos += ret;
-	}
+	to = 0;
+	ret = read_partial(con, &to, sizeof (con->in_hdr), &con->in_hdr);
+	if (ret <= 0)
+		return ret;
 
 	crc = crc32c(0, &con->in_hdr, offsetof(struct ceph_msg_header, crc));
 	if (cpu_to_le32(crc) != con->in_hdr.crc) {

@@ -1759,16 +1754,11 @@ static int read_partial_message(struct ceph_connection *con)
 	}
 
 	/* footer */
-	to = sizeof(m->hdr) + sizeof(m->footer);
-	while (con->in_base_pos < to) {
-		left = to - con->in_base_pos;
-		ret = ceph_tcp_recvmsg(con->sock, (char *)&m->footer +
-				       (con->in_base_pos - sizeof(m->hdr)),
-				       left);
-		if (ret <= 0)
-			return ret;
-		con->in_base_pos += ret;
-	}
+	to = sizeof (m->hdr);
+	ret = read_partial(con, &to, sizeof (m->footer), &m->footer);
+	if (ret <= 0)
+		return ret;
+
 	dout("read_partial_message got msg %p %d (%u) + %d (%u) + %d (%u)\n",
 	     m, front_len, m->footer.front_crc, middle_len,
 	     m->footer.middle_crc, data_len, m->footer.data_crc);

Signature · 1 line

1.7.9.5

Sage Weil

Reviewed-by on v1 1/3

2012-05-12 23:54 UTC #2 lore raw

Re: [PATCH 1/3] ceph: messenger: use read_partial() in read_partial_message()

Reviewed-by Sage Weil <[email protected]>

Quoted 77 lines · Alex Elder

There are two blocks of code in read_partial_message()--those that read the header and footer of the message--that can be replaced by a call to read_partial(). Do that.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

net/ceph/messenger.c +1 -1

index f0993af..673133e 100644
@@ -1628,7 +1628,7 @@ static int read_partial_message(struct ceph_connection
 *con)
 {
 	struct ceph_msg *m = con->in_msg;
 	int ret;
-	int to, left;
+	int to;
 	unsigned front_len, middle_len, data_len;
 	bool do_datacrc = !con->msgr->nocrc;

net/ceph/messenger.c +4 -9

 	int skip;

@@ -1638,15 +1638,10 @@ static int read_partial_message(struct ceph_connection
 *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	while (con->in_base_pos < sizeof(con->in_hdr)) {
-		left = sizeof(con->in_hdr) - con->in_base_pos;
-		ret = ceph_tcp_recvmsg(con->sock,
-				       (char *)&con->in_hdr +
 con->in_base_pos,
-				       left);
-		if (ret <= 0)
-			return ret;
-		con->in_base_pos += ret;
-	}
+	to = 0;
+	ret = read_partial(con, &to, sizeof (con->in_hdr), &con->in_hdr);
+	if (ret <= 0)
+		return ret;

net/ceph/messenger.c +5 ~~-10~~

 	crc = crc32c(0, &con->in_hdr, offsetof(struct ceph_msg_header, crc));
 	if (cpu_to_le32(crc) != con->in_hdr.crc) {

@@ -1759,16 +1754,11 @@ static int read_partial_message(struct ceph_connection
 *con)
 	}
 
 	/* footer */
-	to = sizeof(m->hdr) + sizeof(m->footer);
-	while (con->in_base_pos < to) {
-		left = to - con->in_base_pos;
-		ret = ceph_tcp_recvmsg(con->sock, (char *)&m->footer +
-				       (con->in_base_pos - sizeof(m->hdr)),
-				       left);
-		if (ret <= 0)
-			return ret;
-		con->in_base_pos += ret;
-	}
+	to = sizeof (m->hdr);
+	ret = read_partial(con, &to, sizeof (m->footer), &m->footer);
+	if (ret <= 0)
+		return ret;
+
 	dout("read_partial_message got msg %p %d (%u) + %d (%u) + %d (%u)\n",
 	     m, front_len, m->footer.front_crc, middle_len,

m->footer.middle_crc, data_len, m->footer.data_crc);

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-05-11 01:18 UTC #3 lore raw

PATCH 2/3 ceph: messenger: update "to" in read_partial() caller

In reply to Alex Elder

read_partial() always increases whatever "to" value is supplied by adding the requested size to it. That's the only thing it does with that pointed-to value.

Do that pointer advance in the caller (and then only when the updated value will be subsequently used), and change the "to" parameter to be an in-only and non-pointer value.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

net/ceph/messenger.c +16 ~~-15~~

index 673133e..37fd2ae 100644
@@ -992,11 +992,12 @@ static int prepare_read_message(struct ceph_connection *con)
 
 
 static int read_partial(struct ceph_connection *con,
-			int *to, int size, void *object)
+			int to, int size, void *object)
 {
-	*to += size;
-	while (con->in_base_pos < *to) {
-		int left = *to - con->in_base_pos;
+	int end = to + size;
+
+	while (con->in_base_pos < end) {
+		int left = end - con->in_base_pos;
 		int have = size - left;
 		int ret = ceph_tcp_recvmsg(con->sock, object + have, left);
 		if (ret <= 0)

@@ -1017,14 +1018,16 @@ static int read_partial_banner(struct ceph_connection *con)
 	dout("read_partial_banner %p at %d\n", con, con->in_base_pos);
 
 	/* peer's banner */
-	ret = read_partial(con, &to, strlen(CEPH_BANNER), con->in_banner);
+	ret = read_partial(con, to, strlen(CEPH_BANNER), con->in_banner);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to, sizeof(con->actual_peer_addr),
+	to += strlen(CEPH_BANNER);
+	ret = read_partial(con, to, sizeof(con->actual_peer_addr),
 			   &con->actual_peer_addr);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to, sizeof(con->peer_addr_for_me),
+	to += sizeof(con->actual_peer_addr);
+	ret = read_partial(con, to, sizeof(con->peer_addr_for_me),
 			   &con->peer_addr_for_me);
 	if (ret <= 0)
 		goto out;

@@ -1038,10 +1041,11 @@ static int read_partial_connect(struct ceph_connection *con)
 
 	dout("read_partial_connect %p at %d\n", con, con->in_base_pos);
 
-	ret = read_partial(con, &to, sizeof(con->in_reply), &con->in_reply);
+	ret = read_partial(con, to, sizeof(con->in_reply), &con->in_reply);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to, le32_to_cpu(con->in_reply.authorizer_len),
+	to += sizeof(con->in_reply);
+	ret = read_partial(con, to, le32_to_cpu(con->in_reply.authorizer_len),
 			   con->auth_reply_buf);
 	if (ret <= 0)
 		goto out;

@@ -1491,9 +1495,7 @@ static int process_connect(struct ceph_connection *con)
  */
 static int read_partial_ack(struct ceph_connection *con)
 {
-	int to = 0;
-
-	return read_partial(con, &to, sizeof(con->in_temp_ack),
+	return read_partial(con, 0, sizeof(con->in_temp_ack),
 			    &con->in_temp_ack);
 }

@@ -1638,8 +1640,7 @@ static int read_partial_message(struct ceph_connection *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	to = 0;
-	ret = read_partial(con, &to, sizeof (con->in_hdr), &con->in_hdr);
+	ret = read_partial(con, 0, sizeof (con->in_hdr), &con->in_hdr);
 	if (ret <= 0)
 		return ret;

@@ -1755,7 +1756,7 @@ static int read_partial_message(struct ceph_connection *con)
 
 	/* footer */
 	to = sizeof (m->hdr);
-	ret = read_partial(con, &to, sizeof (m->footer), &m->footer);
+	ret = read_partial(con, to, sizeof (m->footer), &m->footer);
 	if (ret <= 0)
 		return ret;

Signature · 1 line

1.7.9.5

Sage Weil

Reviewed-by on v1 2/3

2012-05-12 23:55 UTC #4 lore raw

Re: [PATCH 2/3] ceph: messenger: update "to" in read_partial() caller

Reviewed-by Sage Weil <[email protected]>

Quoted 113 lines · Alex Elder

read_partial() always increases whatever "to" value is supplied by adding the requested size to it. That's the only thing it does with that pointed-to value.

Do that pointer advance in the caller (and then only when the updated value will be subsequently used), and change the "to" parameter to be an in-only and non-pointer value.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

net/ceph/messenger.c +5 -4

index 673133e..37fd2ae 100644
@@ -992,11 +992,12 @@ static int prepare_read_message(struct ceph_connection
 *con)
 
 
 static int read_partial(struct ceph_connection *con,
-			int *to, int size, void *object)
+			int to, int size, void *object)
 {
-	*to += size;
-	while (con->in_base_pos < *to) {
-		int left = *to - con->in_base_pos;
+	int end = to + size;
+
+	while (con->in_base_pos < end) {
+		int left = end - con->in_base_pos;
 		int have = size - left;
 		int ret = ceph_tcp_recvmsg(con->sock, object + have, left);

net/ceph/messenger.c +5 -3

 		if (ret <= 0)

@@ -1017,14 +1018,16 @@ static int read_partial_banner(struct ceph_connection
 *con)
 	dout("read_partial_banner %p at %d\n", con, con->in_base_pos);
 
 	/* peer's banner */
-	ret = read_partial(con, &to, strlen(CEPH_BANNER), con->in_banner);
+	ret = read_partial(con, to, strlen(CEPH_BANNER), con->in_banner);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to, sizeof(con->actual_peer_addr),
+	to += strlen(CEPH_BANNER);
+	ret = read_partial(con, to, sizeof(con->actual_peer_addr),
 			   &con->actual_peer_addr);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to, sizeof(con->peer_addr_for_me),
+	to += sizeof(con->actual_peer_addr);
+	ret = read_partial(con, to, sizeof(con->peer_addr_for_me),
 			   &con->peer_addr_for_me);
 	if (ret <= 0)

net/ceph/messenger.c +3 -2

 		goto out;

@@ -1038,10 +1041,11 @@ static int read_partial_connect(struct ceph_connection
 *con)
 
 	dout("read_partial_connect %p at %d\n", con, con->in_base_pos);
 
-	ret = read_partial(con, &to, sizeof(con->in_reply), &con->in_reply);
+	ret = read_partial(con, to, sizeof(con->in_reply), &con->in_reply);
 	if (ret <= 0)
 		goto out;
-	ret = read_partial(con, &to,
 le32_to_cpu(con->in_reply.authorizer_len),
+	to += sizeof(con->in_reply);
+	ret = read_partial(con, to, le32_to_cpu(con->in_reply.authorizer_len),
 			   con->auth_reply_buf);

net/ceph/messenger.c +3 -6

 	if (ret <= 0)
 		goto out;

@@ -1491,9 +1495,7 @@ static int process_connect(struct ceph_connection *con)
  */
 static int read_partial_ack(struct ceph_connection *con)
 {
-	int to = 0;
-
-	return read_partial(con, &to, sizeof(con->in_temp_ack),
+	return read_partial(con, 0, sizeof(con->in_temp_ack),
 			    &con->in_temp_ack);
 }

@@ -1638,8 +1640,7 @@ static int read_partial_message(struct ceph_connection
 *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	to = 0;
-	ret = read_partial(con, &to, sizeof (con->in_hdr), &con->in_hdr);
+	ret = read_partial(con, 0, sizeof (con->in_hdr), &con->in_hdr);
 	if (ret <= 0)
 		return ret;

@@ -1755,7 +1756,7 @@ static int read_partial_message(struct ceph_connection
 *con)
 
 	/* footer */
 	to = sizeof (m->hdr);
-	ret = read_partial(con, &to, sizeof (m->footer), &m->footer);
+	ret = read_partial(con, to, sizeof (m->footer), &m->footer);
 	if (ret <= 0)
 		return ret;

Signature · 7 lines

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-05-11 01:18 UTC #5 lore raw

PATCH 3/3 ceph: messenger: change read_partial() to take "end" arg

In reply to Alex Elder

Make the second argument to read_partial() be the ending input byte position rather than the beginning offset it now represents. This amounts to moving the addition "to + size" into the caller.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   59 
++++++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 21 deletions(-)

net/ceph/messenger.c +38 ~~-21~~

index 37fd2ae..364c902 100644
@@ -992,10 +992,8 @@ static int prepare_read_message(struct ceph_connection *con)
 
 
 static int read_partial(struct ceph_connection *con,
-			int to, int size, void *object)
+			int end, int size, void *object)
 {
-	int end = to + size;
-
 	while (con->in_base_pos < end) {
 		int left = end - con->in_base_pos;
 		int have = size - left;

@@ -1013,40 +1011,52 @@ static int read_partial(struct ceph_connection *con,
  */
 static int read_partial_banner(struct ceph_connection *con)
 {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
 	dout("read_partial_banner %p at %d\n", con, con->in_base_pos);
 
 	/* peer's banner */
-	ret = read_partial(con, to, strlen(CEPH_BANNER), con->in_banner);
+	size = strlen(CEPH_BANNER);
+	end = size;
+	ret = read_partial(con, end, size, con->in_banner);
 	if (ret <= 0)
 		goto out;
-	to += strlen(CEPH_BANNER);
-	ret = read_partial(con, to, sizeof(con->actual_peer_addr),
-			   &con->actual_peer_addr);
+
+	size = sizeof (con->actual_peer_addr);
+	end += size;
+	ret = read_partial(con, end, size, &con->actual_peer_addr);
 	if (ret <= 0)
 		goto out;
-	to += sizeof(con->actual_peer_addr);
-	ret = read_partial(con, to, sizeof(con->peer_addr_for_me),
-			   &con->peer_addr_for_me);
+
+	size = sizeof (con->peer_addr_for_me);
+	end += size;
+	ret = read_partial(con, end, size, &con->peer_addr_for_me);
 	if (ret <= 0)
 		goto out;
+
 out:
 	return ret;
 }
 
 static int read_partial_connect(struct ceph_connection *con)
 {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
 	dout("read_partial_connect %p at %d\n", con, con->in_base_pos);
 
-	ret = read_partial(con, to, sizeof(con->in_reply), &con->in_reply);
+	size = sizeof (con->in_reply);
+	end = size;
+	ret = read_partial(con, end, size, &con->in_reply);
 	if (ret <= 0)
 		goto out;
-	to += sizeof(con->in_reply);
-	ret = read_partial(con, to, le32_to_cpu(con->in_reply.authorizer_len),
-			   con->auth_reply_buf);
+
+	size = le32_to_cpu(con->in_reply.authorizer_len);
+	end += size;
+	ret = read_partial(con, end, size, con->auth_reply_buf);
 	if (ret <= 0)
 		goto out;

@@ -1495,8 +1505,10 @@ static int process_connect(struct ceph_connection *con)
  */
 static int read_partial_ack(struct ceph_connection *con)
 {
-	return read_partial(con, 0, sizeof(con->in_temp_ack),
-			    &con->in_temp_ack);
+	int size = sizeof (con->in_temp_ack);
+	int end = size;
+
+	return read_partial(con, end, size, &con->in_temp_ack);
 }

@@ -1629,6 +1641,8 @@ static int read_partial_message_bio(struct ceph_connection *con,
 static int read_partial_message(struct ceph_connection *con)
 {
 	struct ceph_msg *m = con->in_msg;
+	int size;
+	int end;
 	int ret;
 	int to;
 	unsigned front_len, middle_len, data_len;

@@ -1640,7 +1654,9 @@ static int read_partial_message(struct ceph_connection *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	ret = read_partial(con, 0, sizeof (con->in_hdr), &con->in_hdr);
+	size = sizeof (con->in_hdr);
+	end = size;
+	ret = read_partial(con, end, size, &con->in_hdr);
 	if (ret <= 0)
 		return ret;

@@ -1755,8 +1771,9 @@ static int read_partial_message(struct ceph_connection *con)
 	}
 
 	/* footer */
-	to = sizeof (m->hdr);
-	ret = read_partial(con, to, sizeof (m->footer), &m->footer);
+	size = sizeof (m->footer);
+	end += size;
+	ret = read_partial(con, end, size, &m->footer);
 	if (ret <= 0)
 		return ret;

Signature · 1 line

1.7.9.5

Sage Weil

2012-05-13 00:11 UTC #6 lore raw

Re: [PATCH 3/3] ceph: messenger: change read_partial() to take "end" arg

This looks correct, but seems like a more confusing calling convention to me. Before this patch it's basically a (start, len) logical range in the input stream.. after it's (end, len). It also seems to be more code?

sage

Quoted 150 lines · Alex Elder

Make the second argument to read_partial() be the ending input byte position rather than the beginning offset it now represents. This amounts to moving the addition "to + size" into the caller.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   59
++++++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 21 deletions(-)

net/ceph/messenger.c +1 -3

index 37fd2ae..364c902 100644
@@ -992,10 +992,8 @@ static int prepare_read_message(struct ceph_connection
 *con)
 
 
 static int read_partial(struct ceph_connection *con,
-			int to, int size, void *object)
+			int end, int size, void *object)
 {
-	int end = to + size;
-
 	while (con->in_base_pos < end) {
 		int left = end - con->in_base_pos;

net/ceph/messenger.c +31 ~~-15~~

 		int have = size - left;

@@ -1013,40 +1011,52 @@ static int read_partial(struct ceph_connection *con,
  */
 static int read_partial_banner(struct ceph_connection *con)
 {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
 	dout("read_partial_banner %p at %d\n", con, con->in_base_pos);
 
 	/* peer's banner */
-	ret = read_partial(con, to, strlen(CEPH_BANNER), con->in_banner);
+	size = strlen(CEPH_BANNER);
+	end = size;
+	ret = read_partial(con, end, size, con->in_banner);
 	if (ret <= 0)
 		goto out;
-	to += strlen(CEPH_BANNER);
-	ret = read_partial(con, to, sizeof(con->actual_peer_addr),
-			   &con->actual_peer_addr);
+
+	size = sizeof (con->actual_peer_addr);
+	end += size;
+	ret = read_partial(con, end, size, &con->actual_peer_addr);
 	if (ret <= 0)
 		goto out;
-	to += sizeof(con->actual_peer_addr);
-	ret = read_partial(con, to, sizeof(con->peer_addr_for_me),
-			   &con->peer_addr_for_me);
+
+	size = sizeof (con->peer_addr_for_me);
+	end += size;
+	ret = read_partial(con, end, size, &con->peer_addr_for_me);
 	if (ret <= 0)
 		goto out;
+
 out:
 	return ret;
 }
 
 static int read_partial_connect(struct ceph_connection *con)
 {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
 	dout("read_partial_connect %p at %d\n", con, con->in_base_pos);
 
-	ret = read_partial(con, to, sizeof(con->in_reply), &con->in_reply);
+	size = sizeof (con->in_reply);
+	end = size;
+	ret = read_partial(con, end, size, &con->in_reply);
 	if (ret <= 0)
 		goto out;
-	to += sizeof(con->in_reply);
-	ret = read_partial(con, to, le32_to_cpu(con->in_reply.authorizer_len),
-			   con->auth_reply_buf);
+
+	size = le32_to_cpu(con->in_reply.authorizer_len);
+	end += size;
+	ret = read_partial(con, end, size, con->auth_reply_buf);
 	if (ret <= 0)
 		goto out;

@@ -1495,8 +1505,10 @@ static int process_connect(struct ceph_connection *con)
  */
 static int read_partial_ack(struct ceph_connection *con)
 {
-	return read_partial(con, 0, sizeof(con->in_temp_ack),
-			    &con->in_temp_ack);
+	int size = sizeof (con->in_temp_ack);
+	int end = size;
+
+	return read_partial(con, end, size, &con->in_temp_ack);
 }

@@ -1629,6 +1641,8 @@ static int read_partial_message_bio(struct
 ceph_connection *con,
 static int read_partial_message(struct ceph_connection *con)
 {
 	struct ceph_msg *m = con->in_msg;
+	int size;
+	int end;
 	int ret;
 	int to;

net/ceph/messenger.c +6 -3

 	unsigned front_len, middle_len, data_len;

@@ -1640,7 +1654,9 @@ static int read_partial_message(struct ceph_connection
 *con)
 	dout("read_partial_message con %p msg %p\n", con, m);
 
 	/* header */
-	ret = read_partial(con, 0, sizeof (con->in_hdr), &con->in_hdr);
+	size = sizeof (con->in_hdr);
+	end = size;
+	ret = read_partial(con, end, size, &con->in_hdr);
 	if (ret <= 0)
 		return ret;

@@ -1755,8 +1771,9 @@ static int read_partial_message(struct ceph_connection
 *con)
 	}
 
 	/* footer */
-	to = sizeof (m->hdr);
-	ret = read_partial(con, to, sizeof (m->footer), &m->footer);
+	size = sizeof (m->footer);
+	end += size;
+	ret = read_partial(con, end, size, &m->footer);
 	if (ret <= 0)
 		return ret;

Signature · 7 lines

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-05-14 16:26 UTC #7 lore raw

Re: [PATCH 3/3] ceph: messenger: change read_partial() to take "end" arg

Quoted 4 lines · Sage Weil

This looks correct, but seems like a more confusing calling convention to me. Before this patch it's basically a (start, len) logical range in the input stream.. after it's (end, len). It also seems to be more code?

I'm trying to make this code more understandable. I have this ongoing confusion with what the meaning of "to" is. It may be that it was a "start" position, in which case, "start" might be a better name, and maybe just renaming it would have been better.

However, another source of confusion here is what the meaning of this "start" would be versus the meaning of con->in_base_pos. The actual input position is defined by in_base_pos, while "start" would represent the start of the partially-filled portion of a message we're now working on reading. Where that starts is irrelevant, and I found the existence of something called "start" to be distracting and ambiguous. It's a lot easier to wrap my head around it by just providing the end position for the read and leaving out any (other) notion of the "start" position entirely.

So yes, it expands to a bit more code in the caller but the result is something I can more easily understand and therefore believe in. (I also hope that some later refactoring may make whatever excess this produces go away, or be less glaring anyway.)

-Alex

Quoted 155 lines

sage

Quoted 150 lines · Alex Elder

Make the second argument to read_partial() be the ending input byte position rather than the beginning offset it now represents. This amounts to moving the addition "to + size" into the caller.

Signed-off-by Alex Elder<[email protected]>

---
  net/ceph/messenger.c |   59
++++++++++++++++++++++++++++++++------------------
  1 file changed, 38 insertions(+), 21 deletions(-)

net/ceph/messenger.c +1 -3

index 37fd2ae..364c902 100644
@@ -992,10 +992,8 @@ static int prepare_read_message(struct ceph_connection
 *con)
 
 
  static int read_partial(struct ceph_connection *con,
-			int to, int size, void *object)
+			int end, int size, void *object)
  {
-	int end = to + size;
-
  	while (con->in_base_pos<  end) {
  		int left = end - con->in_base_pos;

  		int have = size - left;

net/ceph/messenger.c +31 ~~-15~~

@@ -1013,40 +1011,52 @@ static int read_partial(struct ceph_connection *con,
   */
  static int read_partial_banner(struct ceph_connection *con)
  {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
  	dout("read_partial_banner %p at %d\n", con, con->in_base_pos);
 
  	/* peer's banner */
-	ret = read_partial(con, to, strlen(CEPH_BANNER), con->in_banner);
+	size = strlen(CEPH_BANNER);
+	end = size;
+	ret = read_partial(con, end, size, con->in_banner);
  	if (ret<= 0)
  		goto out;
-	to += strlen(CEPH_BANNER);
-	ret = read_partial(con, to, sizeof(con->actual_peer_addr),
-			&con->actual_peer_addr);
+
+	size = sizeof (con->actual_peer_addr);
+	end += size;
+	ret = read_partial(con, end, size,&con->actual_peer_addr);
  	if (ret<= 0)
  		goto out;
-	to += sizeof(con->actual_peer_addr);
-	ret = read_partial(con, to, sizeof(con->peer_addr_for_me),
-			&con->peer_addr_for_me);
+
+	size = sizeof (con->peer_addr_for_me);
+	end += size;
+	ret = read_partial(con, end, size,&con->peer_addr_for_me);
  	if (ret<= 0)
  		goto out;
+
  out:
  	return ret;
  }
 
  static int read_partial_connect(struct ceph_connection *con)
  {
-	int ret, to = 0;
+	int size;
+	int end;
+	int ret;
 
  	dout("read_partial_connect %p at %d\n", con, con->in_base_pos);
 
-	ret = read_partial(con, to, sizeof(con->in_reply),&con->in_reply);
+	size = sizeof (con->in_reply);
+	end = size;
+	ret = read_partial(con, end, size,&con->in_reply);
  	if (ret<= 0)
  		goto out;
-	to += sizeof(con->in_reply);
-	ret = read_partial(con, to, le32_to_cpu(con->in_reply.authorizer_len),
-			   con->auth_reply_buf);
+
+	size = le32_to_cpu(con->in_reply.authorizer_len);
+	end += size;
+	ret = read_partial(con, end, size, con->auth_reply_buf);
  	if (ret<= 0)
  		goto out;

@@ -1495,8 +1505,10 @@ static int process_connect(struct ceph_connection *con)
   */
  static int read_partial_ack(struct ceph_connection *con)
  {
-	return read_partial(con, 0, sizeof(con->in_temp_ack),
-			&con->in_temp_ack);
+	int size = sizeof (con->in_temp_ack);
+	int end = size;
+
+	return read_partial(con, end, size,&con->in_temp_ack);
  }

@@ -1629,6 +1641,8 @@ static int read_partial_message_bio(struct
 ceph_connection *con,
  static int read_partial_message(struct ceph_connection *con)
  {
  	struct ceph_msg *m = con->in_msg;
+	int size;
+	int end;
  	int ret;
  	int to;

unsigned front_len, middle_len, data_len;

net/ceph/messenger.c +6 -4

@@ -1640,7 +1654,9 @@ static int read_partial_message(struct ceph_connection
 *con)
  	dout("read_partial_message con %p msg %p\n", con, m);
 
  	/* header */
-	ret = read_partial(con, 0, sizeof (con->in_hdr),&con->in_hdr);
+	size = sizeof (con->in_hdr);
+	end = size;
+	ret = read_partial(con, end, size,&con->in_hdr);
  	if (ret<= 0)
  		return ret;

@@ -1755,8 +1771,9 @@ static int read_partial_message(struct ceph_connection
 *con)
  	}
 
  	/* footer */
-	to = sizeof (m->hdr);
-	ret = read_partial(con, to, sizeof (m->footer),&m->footer);
+	size = sizeof (m->footer);
+	end += size;
+	ret = read_partial(con, end, size,&m->footer);
  	if (ret<= 0)
  		return ret;
 
--

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-05-14 16:28 UTC #8 lore raw

Re: [PATCH 3/3] ceph: messenger: change read_partial() to take "end" arg

In reply to Sage Weil

Quoted 5 lines · Sage Weil

This looks correct, but seems like a more confusing calling convention to me. Before this patch it's basically a (start, len) logical range in the input stream.. after it's (end, len). It also seems to be more code?

Quoted 10 lines

sage
Quoted 6 lines · Alex Elder
Make the second argument to read_partial() be the ending input byte position rather than the beginning offset it now represents. This amounts to moving the addition "to + size" into the caller.
Signed-off-by Alex Elder<[email protected]>
---

Note that this patch also left the local variable "to" in read_partial_message() unused. I'll delete its definition before I commit.

Thanks for the review.

-Alex

. . .

Quoted 23 lines

Quoted 22 lines
net/ceph/messenger.c +3 -3
@@ -1755,8 +1771,9 @@ static int read_partial_message(struct ceph_connection
 *con)
  	}
 
  	/* footer */
-	to = sizeof (m->hdr);
-	ret = read_partial(con, to, sizeof (m->footer),&m->footer);
+	size = sizeof (m->footer);
+	end += size;
+	ret = read_partial(con, end, size,&m->footer);
  	if (ret<= 0)
  		return ret;
 
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html