On Mon, May 6, 2019 at 3:38 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> GCC9 is throwing a lot of warnings about unaligned access. This patch
> fixes some of them by changing most of the sockaddr handling functions
> to take a pointer to struct ceph_entity_addr instead of struct
> sockaddr_storage. The lower functions can then make copies or do
> unaligned accesses as needed.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> net/ceph/messenger.c | 75 +++++++++++++++++++++-----------------------
> 1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 3083988ce729..3cb8ce385fce 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -449,7 +449,7 @@ static void set_sock_callbacks(struct socket *sock,
> */
> static int ceph_tcp_connect(struct ceph_connection *con)
> {
> - struct sockaddr_storage *paddr = &con->peer_addr.in_addr;
> + struct sockaddr_storage addr = con->peer_addr.in_addr; /* align */
> struct socket *sock;
> unsigned int noio_flag;
> int ret;
> @@ -458,7 +458,7 @@ static int ceph_tcp_connect(struct ceph_connection *con)
>
> /* sock_create_kern() allocates with GFP_KERNEL */
> noio_flag = memalloc_noio_save();
> - ret = sock_create_kern(read_pnet(&con->msgr->net), paddr->ss_family,
> + ret = sock_create_kern(read_pnet(&con->msgr->net), addr.ss_family,
> SOCK_STREAM, IPPROTO_TCP, &sock);
> memalloc_noio_restore(noio_flag);
> if (ret)
> @@ -474,7 +474,7 @@ static int ceph_tcp_connect(struct ceph_connection *con)
> dout("connect %s\n", ceph_pr_addr(&con->peer_addr.in_addr));
>
> con_sock_state_connecting(con);
> - ret = sock->ops->connect(sock, (struct sockaddr *)paddr, sizeof(*paddr),
> + ret = sock->ops->connect(sock, (struct sockaddr *)&addr, sizeof(addr),
> O_NONBLOCK);
> if (ret == -EINPROGRESS) {
> dout("connect %s EINPROGRESS sk_state = %u\n",
> @@ -1795,12 +1795,13 @@ static int verify_hello(struct ceph_connection *con)
> return 0;
> }
>
> -static bool addr_is_blank(struct sockaddr_storage *ss)
> +static bool addr_is_blank(struct ceph_entity_addr *ea)
> {
> - struct in_addr *addr = &((struct sockaddr_in *)ss)->sin_addr;
> - struct in6_addr *addr6 = &((struct sockaddr_in6 *)ss)->sin6_addr;
> + struct sockaddr_storage ss = ea->in_addr;
> + struct in_addr *addr = &((struct sockaddr_in *)&ss)->sin_addr;
> + struct in6_addr *addr6 = &((struct sockaddr_in6 *)&ss)->sin6_addr;
>
> - switch (ss->ss_family) {
> + switch (ss.ss_family) {
> case AF_INET:
> return addr->s_addr == htonl(INADDR_ANY);
> case AF_INET6:
> @@ -1810,25 +1811,25 @@ static bool addr_is_blank(struct sockaddr_storage *ss)
> }
> }
>
> -static int addr_port(struct sockaddr_storage *ss)
> +static int addr_port(struct ceph_entity_addr *ea)
> {
> - switch (ss->ss_family) {
> + switch (get_unaligned(&ea->in_addr.ss_family)) {
> case AF_INET:
> - return ntohs(((struct sockaddr_in *)ss)->sin_port);
> + return ntohs(get_unaligned(&((struct sockaddr_in *)&ea->in_addr)->sin_port));
> case AF_INET6:
> - return ntohs(((struct sockaddr_in6 *)ss)->sin6_port);
> + return ntohs(get_unaligned(&((struct sockaddr_in6 *)&ea->in_addr)->sin6_port));
> }
> return 0;
> }
This is what I had in mind (I don't have gcc 9 around to play with).
Looks good. I have a couple of naming nits: sockaddr_storage in
ceph_tcp_connect() can be "ss" and ceph_entity_addr * here and in
addr_is_blank() can be "*addr" for consistency with existing code.
I'll fix that and apply.
Thanks,
Ilya
