diff mbox series

[1/8] libceph: store ceph_auth_handshake pointer in ceph_connection

Message ID 20180801190350.857-2-idryomov@gmail.com (mailing list archive)
State New, archived
Headers show
Series libceph: support for cephx v2 | expand

Commit Message

Ilya Dryomov Aug. 1, 2018, 7:03 p.m. UTC 
We already copy authorizer_reply_buf and authorizer_reply_buf_len into
ceph_connection.  Factoring out __prepare_write_connect() requires two
more: authorizer_buf and authorizer_buf_len.  Store the pointer to the
handshake in con->auth rather than piling on.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 include/linux/ceph/messenger.h |  3 +--
 net/ceph/messenger.c           | 54 ++++++++++++++++++++----------------------
 2 files changed, 27 insertions(+), 30 deletions(-)

Comments

Sage Weil Aug. 1, 2018, 7:27 p.m. UTC | #1 
On Wed, 1 Aug 2018, Ilya Dryomov wrote:
> We already copy authorizer_reply_buf and authorizer_reply_buf_len into
> ceph_connection.  Factoring out __prepare_write_connect() requires two
> more: authorizer_buf and authorizer_buf_len.  Store the pointer to the
> handshake in con->auth rather than piling on.
> 
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  include/linux/ceph/messenger.h |  3 +--
>  net/ceph/messenger.c           | 54 ++++++++++++++++++++----------------------
>  2 files changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
> index a718b877c597..021718570b50 100644
> --- a/include/linux/ceph/messenger.h
> +++ b/include/linux/ceph/messenger.h
> @@ -286,9 +286,8 @@ struct ceph_connection {
>  				 attempt for this connection, client */
>  	u32 peer_global_seq;  /* peer's global seq for this connection */
>  
> +	struct ceph_auth_handshake *auth;
>  	int auth_retry;       /* true if we need a newer authorizer */
> -	void *auth_reply_buf;   /* where to put the authorizer reply */
> -	int auth_reply_buf_len;
>  
>  	struct mutex mutex;
>  
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 3f6336248509..b6ebd2cc16a1 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1434,24 +1434,26 @@ static void prepare_write_keepalive(struct ceph_connection *con)
>   * Connection negotiation.
>   */
>  
> -static struct ceph_auth_handshake *get_connect_authorizer(struct ceph_connection *con,
> -						int *auth_proto)
> +static int get_connect_authorizer(struct ceph_connection *con)
>  {
>  	struct ceph_auth_handshake *auth;
> +	int auth_proto;
>  
>  	if (!con->ops->get_authorizer) {
> +		con->auth = NULL;
>  		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
>  		con->out_connect.authorizer_len = 0;
> -		return NULL;
> +		return 0;
>  	}
>  
> -	auth = con->ops->get_authorizer(con, auth_proto, con->auth_retry);
> +	auth = con->ops->get_authorizer(con, &auth_proto, con->auth_retry);
>  	if (IS_ERR(auth))
> -		return auth;
> +		return PTR_ERR(auth);
>  
> -	con->auth_reply_buf = auth->authorizer_reply_buf;
> -	con->auth_reply_buf_len = auth->authorizer_reply_buf_len;
> -	return auth;
> +	con->auth = auth;
> +	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
> +	con->out_connect.authorizer_len = cpu_to_le32(auth->authorizer_buf_len);
> +	return 0;
>  }
>  
>  /*
> @@ -1471,8 +1473,7 @@ static int prepare_write_connect(struct ceph_connection *con)
>  {
>  	unsigned int global_seq = get_global_seq(con->msgr, 0);
>  	int proto;
> -	int auth_proto;
> -	struct ceph_auth_handshake *auth;
> +	int ret;
>  
>  	switch (con->peer_name.type) {
>  	case CEPH_ENTITY_TYPE_MON:
> @@ -1499,20 +1500,15 @@ static int prepare_write_connect(struct ceph_connection *con)
>  	con->out_connect.protocol_version = cpu_to_le32(proto);
>  	con->out_connect.flags = 0;
>  
> -	auth_proto = CEPH_AUTH_UNKNOWN;
> -	auth = get_connect_authorizer(con, &auth_proto);
> -	if (IS_ERR(auth))
> -		return PTR_ERR(auth);
> -
> -	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
> -	con->out_connect.authorizer_len = auth ?
> -		cpu_to_le32(auth->authorizer_buf_len) : 0;
> +	ret = get_connect_authorizer(con);
> +	if (ret)
> +		return ret;
>  
>  	con_out_kvec_add(con, sizeof (con->out_connect),
>  					&con->out_connect);
> -	if (auth && auth->authorizer_buf_len)
> -		con_out_kvec_add(con, auth->authorizer_buf_len,
> -					auth->authorizer_buf);
> +	if (con->auth)
> +		con_out_kvec_add(con, con->auth->authorizer_buf_len,
> +				 con->auth->authorizer_buf);
>  
>  	con->out_more = 0;
>  	con_flag_set(con, CON_FLAG_WRITE_PENDING);
> @@ -1781,11 +1777,14 @@ static int read_partial_connect(struct ceph_connection *con)
>  	if (ret <= 0)
>  		goto out;
>  
> -	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;
> +	if (con->auth) {
> +		size = le32_to_cpu(con->in_reply.authorizer_len);
> +		end += size;
> +		ret = read_partial(con, end, size,
> +				   con->auth->authorizer_reply_buf);
> +		if (ret <= 0)
> +			goto out;
> +	}

Is this still right in the case where auth == NULL?

>  
>  	dout("read_partial_connect %p tag %d, con_seq = %u, g_seq = %u\n",
>  	     con, (int)con->in_reply.tag,
> @@ -1793,7 +1792,6 @@ static int read_partial_connect(struct ceph_connection *con)
>  	     le32_to_cpu(con->in_reply.global_seq));
>  out:
>  	return ret;
> -
>  }
>  
>  /*
> @@ -2076,7 +2074,7 @@ static int process_connect(struct ceph_connection *con)
>  
>  	dout("process_connect on %p tag %d\n", con, (int)con->in_tag);
>  
> -	if (con->auth_reply_buf) {
> +	if (con->auth) {
>  		/*
>  		 * Any connection that defines ->get_authorizer()
>  		 * should also define ->verify_authorizer_reply().
> -- 
> 2.14.4
> 
> --
> 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
> 
> 
--
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 Aug. 1, 2018, 7:41 p.m. UTC | #2 
On Wed, Aug 1, 2018 at 9:27 PM Sage Weil <sage@newdream.net> wrote:
>
> On Wed, 1 Aug 2018, Ilya Dryomov wrote:
> > We already copy authorizer_reply_buf and authorizer_reply_buf_len into
> > ceph_connection.  Factoring out __prepare_write_connect() requires two
> > more: authorizer_buf and authorizer_buf_len.  Store the pointer to the
> > handshake in con->auth rather than piling on.
> >
> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > ---
> >  include/linux/ceph/messenger.h |  3 +--
> >  net/ceph/messenger.c           | 54 ++++++++++++++++++++----------------------
> >  2 files changed, 27 insertions(+), 30 deletions(-)
> >
> > diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
> > index a718b877c597..021718570b50 100644
> > --- a/include/linux/ceph/messenger.h
> > +++ b/include/linux/ceph/messenger.h
> > @@ -286,9 +286,8 @@ struct ceph_connection {
> >                                attempt for this connection, client */
> >       u32 peer_global_seq;  /* peer's global seq for this connection */
> >
> > +     struct ceph_auth_handshake *auth;
> >       int auth_retry;       /* true if we need a newer authorizer */
> > -     void *auth_reply_buf;   /* where to put the authorizer reply */
> > -     int auth_reply_buf_len;
> >
> >       struct mutex mutex;
> >
> > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > index 3f6336248509..b6ebd2cc16a1 100644
> > --- a/net/ceph/messenger.c
> > +++ b/net/ceph/messenger.c
> > @@ -1434,24 +1434,26 @@ static void prepare_write_keepalive(struct ceph_connection *con)
> >   * Connection negotiation.
> >   */
> >
> > -static struct ceph_auth_handshake *get_connect_authorizer(struct ceph_connection *con,
> > -                                             int *auth_proto)
> > +static int get_connect_authorizer(struct ceph_connection *con)
> >  {
> >       struct ceph_auth_handshake *auth;
> > +     int auth_proto;
> >
> >       if (!con->ops->get_authorizer) {
> > +             con->auth = NULL;
> >               con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
> >               con->out_connect.authorizer_len = 0;
> > -             return NULL;
> > +             return 0;
> >       }
> >
> > -     auth = con->ops->get_authorizer(con, auth_proto, con->auth_retry);
> > +     auth = con->ops->get_authorizer(con, &auth_proto, con->auth_retry);
> >       if (IS_ERR(auth))
> > -             return auth;
> > +             return PTR_ERR(auth);
> >
> > -     con->auth_reply_buf = auth->authorizer_reply_buf;
> > -     con->auth_reply_buf_len = auth->authorizer_reply_buf_len;
> > -     return auth;
> > +     con->auth = auth;
> > +     con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
> > +     con->out_connect.authorizer_len = cpu_to_le32(auth->authorizer_buf_len);
> > +     return 0;
> >  }
> >
> >  /*
> > @@ -1471,8 +1473,7 @@ static int prepare_write_connect(struct ceph_connection *con)
> >  {
> >       unsigned int global_seq = get_global_seq(con->msgr, 0);
> >       int proto;
> > -     int auth_proto;
> > -     struct ceph_auth_handshake *auth;
> > +     int ret;
> >
> >       switch (con->peer_name.type) {
> >       case CEPH_ENTITY_TYPE_MON:
> > @@ -1499,20 +1500,15 @@ static int prepare_write_connect(struct ceph_connection *con)
> >       con->out_connect.protocol_version = cpu_to_le32(proto);
> >       con->out_connect.flags = 0;
> >
> > -     auth_proto = CEPH_AUTH_UNKNOWN;
> > -     auth = get_connect_authorizer(con, &auth_proto);
> > -     if (IS_ERR(auth))
> > -             return PTR_ERR(auth);
> > -
> > -     con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
> > -     con->out_connect.authorizer_len = auth ?
> > -             cpu_to_le32(auth->authorizer_buf_len) : 0;
> > +     ret = get_connect_authorizer(con);
> > +     if (ret)
> > +             return ret;
> >
> >       con_out_kvec_add(con, sizeof (con->out_connect),
> >                                       &con->out_connect);
> > -     if (auth && auth->authorizer_buf_len)
> > -             con_out_kvec_add(con, auth->authorizer_buf_len,
> > -                                     auth->authorizer_buf);
> > +     if (con->auth)
> > +             con_out_kvec_add(con, con->auth->authorizer_buf_len,
> > +                              con->auth->authorizer_buf);
> >
> >       con->out_more = 0;
> >       con_flag_set(con, CON_FLAG_WRITE_PENDING);
> > @@ -1781,11 +1777,14 @@ static int read_partial_connect(struct ceph_connection *con)
> >       if (ret <= 0)
> >               goto out;
> >
> > -     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;
> > +     if (con->auth) {
> > +             size = le32_to_cpu(con->in_reply.authorizer_len);
> > +             end += size;
> > +             ret = read_partial(con, end, size,
> > +                                con->auth->authorizer_reply_buf);
> > +             if (ret <= 0)
> > +                     goto out;
> > +     }
>
> Is this still right in the case where auth == NULL?

You mean con->auth (i.e. the handshake)?  If con->auth is NULL, there
shouldn't be any authorizer replies coming our way.

Previously read_partial() would get called with NULL for @object, but
with size == 0 it just bails immediately.  The change is that I don't
bother calling read_partial() if there is no auth.

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 mbox series

Patch

diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index a718b877c597..021718570b50 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -286,9 +286,8 @@  struct ceph_connection {
 				 attempt for this connection, client */
 	u32 peer_global_seq;  /* peer's global seq for this connection */
 
+	struct ceph_auth_handshake *auth;
 	int auth_retry;       /* true if we need a newer authorizer */
-	void *auth_reply_buf;   /* where to put the authorizer reply */
-	int auth_reply_buf_len;
 
 	struct mutex mutex;
 
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 3f6336248509..b6ebd2cc16a1 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1434,24 +1434,26 @@  static void prepare_write_keepalive(struct ceph_connection *con)
  * Connection negotiation.
  */
 
-static struct ceph_auth_handshake *get_connect_authorizer(struct ceph_connection *con,
-						int *auth_proto)
+static int get_connect_authorizer(struct ceph_connection *con)
 {
 	struct ceph_auth_handshake *auth;
+	int auth_proto;
 
 	if (!con->ops->get_authorizer) {
+		con->auth = NULL;
 		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
 		con->out_connect.authorizer_len = 0;
-		return NULL;
+		return 0;
 	}
 
-	auth = con->ops->get_authorizer(con, auth_proto, con->auth_retry);
+	auth = con->ops->get_authorizer(con, &auth_proto, con->auth_retry);
 	if (IS_ERR(auth))
-		return auth;
+		return PTR_ERR(auth);
 
-	con->auth_reply_buf = auth->authorizer_reply_buf;
-	con->auth_reply_buf_len = auth->authorizer_reply_buf_len;
-	return auth;
+	con->auth = auth;
+	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
+	con->out_connect.authorizer_len = cpu_to_le32(auth->authorizer_buf_len);
+	return 0;
 }
 
 /*
@@ -1471,8 +1473,7 @@  static int prepare_write_connect(struct ceph_connection *con)
 {
 	unsigned int global_seq = get_global_seq(con->msgr, 0);
 	int proto;
-	int auth_proto;
-	struct ceph_auth_handshake *auth;
+	int ret;
 
 	switch (con->peer_name.type) {
 	case CEPH_ENTITY_TYPE_MON:
@@ -1499,20 +1500,15 @@  static int prepare_write_connect(struct ceph_connection *con)
 	con->out_connect.protocol_version = cpu_to_le32(proto);
 	con->out_connect.flags = 0;
 
-	auth_proto = CEPH_AUTH_UNKNOWN;
-	auth = get_connect_authorizer(con, &auth_proto);
-	if (IS_ERR(auth))
-		return PTR_ERR(auth);
-
-	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
-	con->out_connect.authorizer_len = auth ?
-		cpu_to_le32(auth->authorizer_buf_len) : 0;
+	ret = get_connect_authorizer(con);
+	if (ret)
+		return ret;
 
 	con_out_kvec_add(con, sizeof (con->out_connect),
 					&con->out_connect);
-	if (auth && auth->authorizer_buf_len)
-		con_out_kvec_add(con, auth->authorizer_buf_len,
-					auth->authorizer_buf);
+	if (con->auth)
+		con_out_kvec_add(con, con->auth->authorizer_buf_len,
+				 con->auth->authorizer_buf);
 
 	con->out_more = 0;
 	con_flag_set(con, CON_FLAG_WRITE_PENDING);
@@ -1781,11 +1777,14 @@  static int read_partial_connect(struct ceph_connection *con)
 	if (ret <= 0)
 		goto out;
 
-	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;
+	if (con->auth) {
+		size = le32_to_cpu(con->in_reply.authorizer_len);
+		end += size;
+		ret = read_partial(con, end, size,
+				   con->auth->authorizer_reply_buf);
+		if (ret <= 0)
+			goto out;
+	}
 
 	dout("read_partial_connect %p tag %d, con_seq = %u, g_seq = %u\n",
 	     con, (int)con->in_reply.tag,
@@ -1793,7 +1792,6 @@  static int read_partial_connect(struct ceph_connection *con)
 	     le32_to_cpu(con->in_reply.global_seq));
 out:
 	return ret;
-
 }
 
 /*
@@ -2076,7 +2074,7 @@  static int process_connect(struct ceph_connection *con)
 
 	dout("process_connect on %p tag %d\n", con, (int)con->in_tag);
 
-	if (con->auth_reply_buf) {
+	if (con->auth) {
 		/*
 		 * Any connection that defines ->get_authorizer()
 		 * should also define ->verify_authorizer_reply().