[2/4] libceph: use the right footer size when skipping a message
diff mbox

Message ID 1455986729-12544-3-git-send-email-idryomov@gmail.com
State New
Headers show

Commit Message

Ilya Dryomov Feb. 20, 2016, 4:45 p.m. UTC
ceph_msg_footer is 21 bytes long, while ceph_msg_footer_old is only 13.
Don't skip too much when CEPH_FEATURE_MSG_AUTH isn't negotiated.

Cc: stable@vger.kernel.org # 3.19+
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/messenger.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Alex Elder Feb. 20, 2016, 6:44 p.m. UTC | #1
+On 02/20/2016 10:45 AM, Ilya Dryomov wrote:
> ceph_msg_footer is 21 bytes long, while ceph_msg_footer_old is only 13.
> Don't skip too much when CEPH_FEATURE_MSG_AUTH isn't negotiated.

This looks good, but I have a few suggestions.

You could done some factoring in prepare_write_message_footer()
to use the new function for setting iov_len and updating
out_kvec_bytes.

Consider my suggestions, but otherwise I believe this
is correct.

Reviewed-by: Alex Elder <elder@linaro.org>

> Cc: stable@vger.kernel.org # 3.19+
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  net/ceph/messenger.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index fec20819a5ea..dd7c1b7f932b 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -2284,6 +2284,13 @@ static int read_partial_msg_data(struct ceph_connection *con)
>  	return 1;	/* must return > 0 to indicate success */
>  }
>  
> +static size_t sizeof_footer(struct ceph_connection *con)

I understand why this is named this way.  But it's supplying a
connection as argument, but a footer is a message attribute.
So I might call this con_msg_footer_size().

> +{
> +	return (con->peer_features & CEPH_FEATURE_MSG_AUTH) ?
> +	    sizeof(struct ceph_msg_footer) :
> +	    sizeof(struct ceph_msg_footer_old);
> +}
> +
>  /*
>   * read (part of) a message.
>   */
> @@ -2335,7 +2342,7 @@ static int read_partial_message(struct ceph_connection *con)
>  			ceph_pr_addr(&con->peer_addr.in_addr),
>  			seq, con->in_seq + 1);
>  		con->in_base_pos = -front_len - middle_len - data_len -
> -			sizeof(m->footer);
> +			sizeof_footer(con);
>  		con->in_tag = CEPH_MSGR_TAG_READY;
>  		return 1;
>  	} else if ((s64)seq - (s64)con->in_seq > 1) {
> @@ -2360,7 +2367,7 @@ static int read_partial_message(struct ceph_connection *con)
>  			/* skip this message */
>  			dout("alloc_msg said skip message\n");
>  			con->in_base_pos = -front_len - middle_len - data_len -
> -				sizeof(m->footer);
> +				sizeof_footer(con);
>  			con->in_tag = CEPH_MSGR_TAG_READY;
>  			con->in_seq++;
>  			return 1;
> @@ -2402,11 +2409,7 @@ static int read_partial_message(struct ceph_connection *con)
>  	}
>  
>  	/* footer */
> -	if (need_sign)
> -		size = sizeof(m->footer);
> -	else
> -		size = sizeof(m->old_footer);
> -
> +	size = sizeof_footer(con);
>  	end += size;
>  	ret = read_partial(con, end, size, &m->footer);
>  	if (ret <= 0)
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ilya Dryomov Feb. 20, 2016, 8:05 p.m. UTC | #2
On Sat, Feb 20, 2016 at 7:44 PM, Alex Elder <elder@ieee.org> wrote:
> +On 02/20/2016 10:45 AM, Ilya Dryomov wrote:
>> ceph_msg_footer is 21 bytes long, while ceph_msg_footer_old is only 13.
>> Don't skip too much when CEPH_FEATURE_MSG_AUTH isn't negotiated.
>
> This looks good, but I have a few suggestions.
>
> You could done some factoring in prepare_write_message_footer()
> to use the new function for setting iov_len and updating
> out_kvec_bytes.

I considered it, but we'd still need the if on MSG_AUTH bit, so
I decided it wasn't worth it.  TBH I'm more inclined to rip this
old_footer stuff entirely - it's supported since v0.55, that's
pre-bobtail...

>
> Consider my suggestions, but otherwise I believe this
> is correct.
>
> Reviewed-by: Alex Elder <elder@linaro.org>
>
>> Cc: stable@vger.kernel.org # 3.19+
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>>  net/ceph/messenger.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>> index fec20819a5ea..dd7c1b7f932b 100644
>> --- a/net/ceph/messenger.c
>> +++ b/net/ceph/messenger.c
>> @@ -2284,6 +2284,13 @@ static int read_partial_msg_data(struct ceph_connection *con)
>>       return 1;       /* must return > 0 to indicate success */
>>  }
>>
>> +static size_t sizeof_footer(struct ceph_connection *con)
>
> I understand why this is named this way.  But it's supplying a
> connection as argument, but a footer is a message attribute.
> So I might call this con_msg_footer_size().

The *size* of footer is a connection attribute though ;)

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Elder Feb. 20, 2016, 8:15 p.m. UTC | #3
On 02/20/2016 02:05 PM, Ilya Dryomov wrote:
>>> >>
>>> >> +static size_t sizeof_footer(struct ceph_connection *con)
>> >
>> > I understand why this is named this way.  But it's supplying a
>> > connection as argument, but a footer is a message attribute.
>> > So I might call this con_msg_footer_size().
> The *size* of footer is a connection attribute though ;)

Yeah, I know.  That's why I suggested the con_msg_***().
But in any case, the code looks correct regardless of the
name.

					-Alex
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ilya Dryomov Feb. 21, 2016, 2:31 p.m. UTC | #4
On Sat, Feb 20, 2016 at 9:05 PM, Ilya Dryomov <idryomov@gmail.com> wrote:
> On Sat, Feb 20, 2016 at 7:44 PM, Alex Elder <elder@ieee.org> wrote:
>> +On 02/20/2016 10:45 AM, Ilya Dryomov wrote:
>>> ceph_msg_footer is 21 bytes long, while ceph_msg_footer_old is only 13.
>>> Don't skip too much when CEPH_FEATURE_MSG_AUTH isn't negotiated.
>>
>> This looks good, but I have a few suggestions.
>>
>> You could done some factoring in prepare_write_message_footer()
>> to use the new function for setting iov_len and updating
>> out_kvec_bytes.
>
> I considered it, but we'd still need the if on MSG_AUTH bit, so
> I decided it wasn't worth it.  TBH I'm more inclined to rip this
> old_footer stuff entirely - it's supported since v0.55, that's
> pre-bobtail...

What I missed yesterday was that using sizeof_footer() makes it
possible to use con_out_kvec_add().  I updated 4/4 and also pulled the
need_sign -> sizeof_footer() hunk from 2/4 into it.

Thanks,

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

Patch
diff mbox

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index fec20819a5ea..dd7c1b7f932b 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2284,6 +2284,13 @@  static int read_partial_msg_data(struct ceph_connection *con)
 	return 1;	/* must return > 0 to indicate success */
 }
 
+static size_t sizeof_footer(struct ceph_connection *con)
+{
+	return (con->peer_features & CEPH_FEATURE_MSG_AUTH) ?
+	    sizeof(struct ceph_msg_footer) :
+	    sizeof(struct ceph_msg_footer_old);
+}
+
 /*
  * read (part of) a message.
  */
@@ -2335,7 +2342,7 @@  static int read_partial_message(struct ceph_connection *con)
 			ceph_pr_addr(&con->peer_addr.in_addr),
 			seq, con->in_seq + 1);
 		con->in_base_pos = -front_len - middle_len - data_len -
-			sizeof(m->footer);
+			sizeof_footer(con);
 		con->in_tag = CEPH_MSGR_TAG_READY;
 		return 1;
 	} else if ((s64)seq - (s64)con->in_seq > 1) {
@@ -2360,7 +2367,7 @@  static int read_partial_message(struct ceph_connection *con)
 			/* skip this message */
 			dout("alloc_msg said skip message\n");
 			con->in_base_pos = -front_len - middle_len - data_len -
-				sizeof(m->footer);
+				sizeof_footer(con);
 			con->in_tag = CEPH_MSGR_TAG_READY;
 			con->in_seq++;
 			return 1;
@@ -2402,11 +2409,7 @@  static int read_partial_message(struct ceph_connection *con)
 	}
 
 	/* footer */
-	if (need_sign)
-		size = sizeof(m->footer);
-	else
-		size = sizeof(m->old_footer);
-
+	size = sizeof_footer(con);
 	end += size;
 	ret = read_partial(con, end, size, &m->footer);
 	if (ret <= 0)