| Message ID | 1455986729-12544-3-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+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
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
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
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
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)
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(-)