[libvirt] [PATCH] addrToString: give better error message

The user probably doesn't care what the gai error numbers are, as much as what the failed conversion IP address was. * src/remote/remote_driver.c (addrToString): Mention which address could not be converted. * daemon/remote.c (addrToString): Likewise. --- In response to https://www.redhat.com/archives/libvir-list/2010-April/msg00219.html daemon/remote.c | 24 +++++++++++++++++------- src/remote/remote_driver.c | 22 ++++++++++++++++------ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 149176f..093e9c6 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1,7 +1,7 @@ /* * remote.c: handlers for RPC method calls * - * Copyright (C) 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -41,6 +41,7 @@ #include <string.h> #include <errno.h> #include <fnmatch.h> +#include <arpa/inet.h> #include "virterror_internal.h" #if HAVE_POLKIT0 @@ -3169,21 +3170,30 @@ remoteDispatchAuthList (struct qemud_server *server, #if HAVE_SASL /* - * NB, keep in sync with similar method in src/remote_internal.c + * NB, keep in sync with similar method in src/remote/remote_driver.c */ static char *addrToString(remote_error *rerr, - struct sockaddr_storage *sa, socklen_t salen) { + struct sockaddr_storage *ss, socklen_t salen) { char host[1024], port[20]; char *addr; int err; + struct sockaddr *sa = (struct sockaddr *)ss; - if ((err = getnameinfo((struct sockaddr *)sa, salen, + if ((err = getnameinfo(sa, salen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteDispatchFormatError(rerr, - _("Cannot resolve address %d: %s"), - err, gai_strerror(err)); + char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, sa->sa_data, ip, sizeof ip)) { + remoteDispatchFormatError(rerr, + _("Cannot resolve address %s: %s"), + ip, gai_strerror(err)); + } else { + remoteDispatchFormatError(rerr, + _("Cannot resolve address: %s"), + gai_strerror(err)); + } return NULL; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 44d8c26..752964f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -32,6 +32,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <arpa/inet.h> /* Windows socket compatibility functions. */ #include <errno.h> @@ -6570,21 +6571,30 @@ remoteAuthenticate (virConnectPtr conn, struct private_data *priv, #if HAVE_SASL /* - * NB, keep in sync with similar method in remote/remote.c + * NB, keep in sync with similar method in daemon/remote.c */ -static char *addrToString(struct sockaddr_storage *sa, socklen_t salen) +static char *addrToString(struct sockaddr_storage *ss, socklen_t salen) { char host[NI_MAXHOST], port[NI_MAXSERV]; char *addr; int err; + struct sockaddr *sa = (struct sockaddr *)ss; - if ((err = getnameinfo((struct sockaddr *)sa, salen, + if ((err = getnameinfo(sa, salen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address %d: %s"), - err, gai_strerror(err)); + char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, sa->sa_data, ip, sizeof ip)) { + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot resolve address %s: %s"), + ip, gai_strerror(err)); + } else { + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot resolve address: %s"), + gai_strerror(err)); + } return NULL; } -- 1.6.6.1

2010/4/20 Eric Blake <eblake@redhat.com>:
The user probably doesn't care what the gai error numbers are, as much as what the failed conversion IP address was.
* src/remote/remote_driver.c (addrToString): Mention which address could not be converted. * daemon/remote.c (addrToString): Likewise. ---
In response to https://www.redhat.com/archives/libvir-list/2010-April/msg00219.html
daemon/remote.c | 24 +++++++++++++++++------- src/remote/remote_driver.c | 22 ++++++++++++++++------ 2 files changed, 33 insertions(+), 13 deletions(-)
@@ -3169,21 +3170,30 @@ remoteDispatchAuthList (struct qemud_server *server,
#if HAVE_SASL /* - * NB, keep in sync with similar method in src/remote_internal.c + * NB, keep in sync with similar method in src/remote/remote_driver.c
Nice cleanup.
*/ static char *addrToString(remote_error *rerr, - struct sockaddr_storage *sa, socklen_t salen) { + struct sockaddr_storage *ss, socklen_t salen) { char host[1024], port[20];
Maybe change this to char host[NI_MAXHOST], port[NI_MAXSERV]; like in addrToString in src/remote/remote_driver.c. ACK Matthias

On 04/20/2010 10:13 AM, Eric Blake wrote:
The user probably doesn't care what the gai error numbers are, as much as what the failed conversion IP address was.
+ char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, sa->sa_data, ip, sizeof ip)) {
This is wrong. Given typical layout, sa->sa_data is not the same address as ((sockaddr_in*)sa)->sin_addr or ((sockaddr_in6*)sa)->in6_addr, and since those are at different offsets, the code needs to be conditional on the value of sa->sa_family. I'll post a respin soon. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

The user probably doesn't care what the gai error numbers are, as much as what the failed conversion IP address was. * src/remote/remote_driver.c (addrToString): Mention which address could not be converted. * daemon/remote.c (addrToString): Likewise. --- Changes in v2: Remove magic numbers in remote.c's array sizing Use correct offset for IP addresses daemon/remote.c | 30 ++++++++++++++++++++++-------- src/remote/remote_driver.c | 26 ++++++++++++++++++++------ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 149176f..b753a4a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1,7 +1,7 @@ /* * remote.c: handlers for RPC method calls * - * Copyright (C) 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -41,6 +41,7 @@ #include <string.h> #include <errno.h> #include <fnmatch.h> +#include <arpa/inet.h> #include "virterror_internal.h" #if HAVE_POLKIT0 @@ -3169,21 +3170,34 @@ remoteDispatchAuthList (struct qemud_server *server, #if HAVE_SASL /* - * NB, keep in sync with similar method in src/remote_internal.c + * NB, keep in sync with similar method in src/remote/remote_driver.c */ static char *addrToString(remote_error *rerr, - struct sockaddr_storage *sa, socklen_t salen) { - char host[1024], port[20]; + struct sockaddr_storage *ss, socklen_t salen) { + char host[NI_MAXHOST], port[NI_MAXSERV]; char *addr; int err; + struct sockaddr *sa = (struct sockaddr *)ss; - if ((err = getnameinfo((struct sockaddr *)sa, salen, + if ((err = getnameinfo(sa, salen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteDispatchFormatError(rerr, - _("Cannot resolve address %d: %s"), - err, gai_strerror(err)); + char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, + (sa->sa_family == AF_INET + ? (void *)&((struct sockaddr_in *)sa)->sin_addr + : (void *)&((struct sockaddr_in6 *)sa)->sin6_addr), + ip, sizeof ip)) { + remoteDispatchFormatError(rerr, + _("Cannot resolve address %s: %s"), + ip, gai_strerror(err)); + } else { + remoteDispatchFormatError(rerr, + _("Cannot resolve address: %s"), + gai_strerror(err)); + } return NULL; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 44d8c26..9b3301f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -32,6 +32,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <arpa/inet.h> /* Windows socket compatibility functions. */ #include <errno.h> @@ -6570,21 +6571,34 @@ remoteAuthenticate (virConnectPtr conn, struct private_data *priv, #if HAVE_SASL /* - * NB, keep in sync with similar method in remote/remote.c + * NB, keep in sync with similar method in daemon/remote.c */ -static char *addrToString(struct sockaddr_storage *sa, socklen_t salen) +static char *addrToString(struct sockaddr_storage *ss, socklen_t salen) { char host[NI_MAXHOST], port[NI_MAXSERV]; char *addr; int err; + struct sockaddr *sa = (struct sockaddr *)ss; - if ((err = getnameinfo((struct sockaddr *)sa, salen, + if ((err = getnameinfo(sa, salen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address %d: %s"), - err, gai_strerror(err)); + char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, + (sa->sa_family == AF_INET + ? (void *)&((struct sockaddr_in *)sa)->sin_addr + : (void *)&((struct sockaddr_in6 *)sa)->sin6_addr), + ip, sizeof ip)) { + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot resolve address %s: %s"), + ip, gai_strerror(err)); + } else { + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot resolve address: %s"), + gai_strerror(err)); + } return NULL; } -- 1.6.6.1

2010/4/21 Eric Blake <eblake@redhat.com>:
The user probably doesn't care what the gai error numbers are, as much as what the failed conversion IP address was.
* src/remote/remote_driver.c (addrToString): Mention which address could not be converted. * daemon/remote.c (addrToString): Likewise. ---
Changes in v2: Remove magic numbers in remote.c's array sizing Use correct offset for IP addresses
daemon/remote.c | 30 ++++++++++++++++++++++-------- src/remote/remote_driver.c | 26 ++++++++++++++++++++------ 2 files changed, 42 insertions(+), 14 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 149176f..b753a4a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1,7 +1,7 @@ /* * remote.c: handlers for RPC method calls * - * Copyright (C) 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -41,6 +41,7 @@ #include <string.h> #include <errno.h> #include <fnmatch.h> +#include <arpa/inet.h> #include "virterror_internal.h"
#if HAVE_POLKIT0 @@ -3169,21 +3170,34 @@ remoteDispatchAuthList (struct qemud_server *server,
#if HAVE_SASL /* - * NB, keep in sync with similar method in src/remote_internal.c + * NB, keep in sync with similar method in src/remote/remote_driver.c */ static char *addrToString(remote_error *rerr, - struct sockaddr_storage *sa, socklen_t salen) { - char host[1024], port[20]; + struct sockaddr_storage *ss, socklen_t salen) { + char host[NI_MAXHOST], port[NI_MAXSERV]; char *addr; int err; + struct sockaddr *sa = (struct sockaddr *)ss;
- if ((err = getnameinfo((struct sockaddr *)sa, salen, + if ((err = getnameinfo(sa, salen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteDispatchFormatError(rerr, - _("Cannot resolve address %d: %s"), - err, gai_strerror(err)); + char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, + (sa->sa_family == AF_INET + ? (void *)&((struct sockaddr_in *)sa)->sin_addr + : (void *)&((struct sockaddr_in6 *)sa)->sin6_addr),
Could we avoid this void * cast? The same cast is in src/remote/remote_driver.c too.
+ ip, sizeof ip)) { + remoteDispatchFormatError(rerr, + _("Cannot resolve address %s: %s"), + ip, gai_strerror(err)); + } else { + remoteDispatchFormatError(rerr, + _("Cannot resolve address: %s"), + gai_strerror(err)); + } return NULL; }
ACK. Matthias

On 04/22/2010 02:33 PM, Matthias Bolte wrote:
+ char ip[INET6_ADDRSTRLEN]; + + if (inet_ntop(sa->sa_family, + (sa->sa_family == AF_INET + ? (void *)&((struct sockaddr_in *)sa)->sin_addr + : (void *)&((struct sockaddr_in6 *)sa)->sin6_addr),
Could we avoid this void * cast? The same cast is in src/remote/remote_driver.c too.
Yeah, I thought it was pretty ugly; gcc is (rightfully) complaining (which in turn failed with -Werror) that sin_addr and sin6_addr are different types, and therefore warned that the ?: had issues without at least one of the two casts to void*. I pushed, after dropping ?: and instead using: void *rawaddr; if (sa->sa_family == AF_INET) rawaddr = &((struct sockaddr_in *)sa)->sin_addr; else rawaddr = &((struct sockaddr_in6 *)sa)->sin6_addr; if (inet_ntop(sa->sa_family, rawaddr, ip, sizeof ip)) {
ACK.
Thanks. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Matthias Bolte