David Lutterkort <lutter(a)redhat.com&gt; wrote: Hi David, ACK, modulo some nits: ... While all the C code uses spaces for indentation, this file uses TABs, so using spaces here looks weird when quoted. ... ... Avoid duplicating constants like that: s/HOST_NAME_MAX+1/sizeof hostname/

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: Good point. Yet another reason not to use gethostbyname. I think it's time to make the prohibition official. Here's a patch to do that: From: Jim Meyering <meyering(a)redhat.com&gt; Date: Thu, 11 Dec 2008 11:49:40 +0100 Subject: [PATCH] syntax-check: prohibit all new uses of gethostby* functions * Makefile.maint (sc_prohibit_gethostby): New rule. * .x-sc_prohibit_gethostby: Exempt the few existing uses. --- .x-sc_prohibit_gethostby | 3 +++ Makefile.maint | 6 ++++++ 2 files changed, 9 insertions(+), 0 deletions(-) create mode 100644 .x-sc_prohibit_gethostby diff --git a/.x-sc_prohibit_gethostby b/.x-sc_prohibit_gethostby new file mode 100644 index 0000000..d74ea40 --- /dev/null +++ b/.x-sc_prohibit_gethostby @@ -0,0 +1,3 @@ +gnulib/lib/getaddrinfo.c +gnulib/m4/getaddrinfo.m4 +src/xend_internal.c diff --git a/Makefile.maint b/Makefile.maint index 10d481b..051e4c0 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -390,6 +390,12 @@ sc_prohibit_virBufferAdd_with_string_literal: { echo '$(ME): use virBufferAddLit, not virBufferAdd,' \ 'with a string literal' 1>&2; exit 1; } || : +# Not only do they fail to deal well with ipv6, but the gethostby* +# functions are also not thread-safe. +sc_prohibit_gethostby: + @grep -nE '\<gethostby(addr|name2?) *\(' $$($(VC_LIST_EXCEPT)) && \ + { echo '$(ME): use getaddrinfo, not gethostby*' 1>&2; exit 1; } || : + # Avoid useless parentheses like those in this example: # #if defined (SYMBOL) || defined (SYM2) sc_useless_cpp_parens: -- 1.6.0.4.1044.g77718

Here is a revised patch that addresses all the comments from Jim and Dan: * Use getaddrinfo instead of gethostbyname * Terminate hostname explicitly with NUL_TERMINATE * Do not repeat constant HOST_NAME_MAX+1 * Use tabs, not spaces in libvirt_sym.version.in David From: David Lutterkort <lutter(a)redhat.com&gt; Date: Wed, 10 Dec 2008 18:34:39 -0800 Subject: [PATCH] virConnectGetHostname: return a fully qualified hostname Instead of doing the equivalent of 'hostname', do 'hostname --fqdn', i.e. try to qualify the local host name by an additional call to getaddrinfo(3) --- src/libvirt_sym.version.in | 1 + src/qemu_driver.c | 16 ++++------------ src/test.c | 16 +++++----------- src/uml_driver.c | 15 ++++----------- src/util.c | 33 +++++++++++++++++++++++++++++++++ src/util.h | 2 ++ src/xen_unified.c | 15 +++++---------- 7 files changed, 54 insertions(+), 44 deletions(-) diff --git a/src/libvirt_sym.version.in b/src/libvirt_sym.version.in index de0bc4a..71a47b7 100644 --- a/src/libvirt_sym.version.in +++ b/src/libvirt_sym.version.in @@ -594,6 +594,7 @@ LIBVIRT_PRIVATE_@VERSION@ { virFileReadLimFD; virFileReadPid; virFileLinkPointsTo; + virGetHostname; virParseNumber; virRun; virSkipSpaces; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 5f6fbd1..d6b7515 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1562,23 +1562,16 @@ cleanup: static char * qemudGetHostname (virConnectPtr conn) { - int r; - char hostname[HOST_NAME_MAX+1], *str; + char *result; - r = gethostname (hostname, HOST_NAME_MAX+1); - if (r == -1) { + result = virGetHostname(); + if (result == NULL) { qemudReportError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } /* Caller frees this string. */ - str = strdup (hostname); - if (str == NULL) { - qemudReportError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); - return NULL; - } - return str; + return result; } static int qemudListDomains(virConnectPtr conn, int *ids, int nids) { @@ -4249,4 +4242,3 @@ int qemuRegister(void) { virRegisterStateDriver(&qemuStateDriver); return 0; } - diff --git a/src/test.c b/src/test.c index 257fc8a..cb623ed 100644 --- a/src/test.c +++ b/src/test.c @@ -651,22 +651,16 @@ static int testGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, static char *testGetHostname (virConnectPtr conn) { - int r; - char hostname [HOST_NAME_MAX+1], *str; + char *result; - r = gethostname (hostname, HOST_NAME_MAX+1); - if (r == -1) { + result = virGetHostname(); + if (result == NULL) { testError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } - str = strdup (hostname); - if (str == NULL) { - testError (conn, VIR_ERR_SYSTEM_ERROR, "%s", - strerror (errno)); - return NULL; - } - return str; + /* Caller frees this string. */ + return result; } static int testGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, diff --git a/src/uml_driver.c b/src/uml_driver.c index 408096e..52a4e5b 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -1150,23 +1150,16 @@ cleanup: static char * umlGetHostname (virConnectPtr conn) { - int r; - char hostname[HOST_NAME_MAX+1], *str; + char *result; - r = gethostname (hostname, HOST_NAME_MAX+1); - if (r == -1) { + result = virGetHostname(); + if (result == NULL) { umlReportError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } /* Caller frees this string. */ - str = strdup (hostname); - if (str == NULL) { - umlReportError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); - return NULL; - } - return str; + return result; } static int umlListDomains(virConnectPtr conn, int *ids, int nids) { diff --git a/src/util.c b/src/util.c index da26009..c5e406b 100644 --- a/src/util.c +++ b/src/util.c @@ -47,6 +47,7 @@ #ifdef HAVE_PATHS_H #include <paths.h> #endif +#include <netdb.h> #include "virterror_internal.h" #include "logging.h" @@ -1306,3 +1307,35 @@ int virDiskNameToIndex(const char *name) { return idx; } + +#ifndef AI_CANONIDN +#define AI_CANONIDN 0 +#endif + +char *virGetHostname(void) +{ + int r; + char hostname[HOST_NAME_MAX+1], *result; + struct addrinfo hints, *info; + + r = gethostname (hostname, sizeof(hostname)); + if (r == -1) + return NULL; + NUL_TERMINATE(hostname); + + memset(&hints, 0, sizeof(hints)); + hints.ai_flags = AI_CANONNAME|AI_CANONIDN; + hints.ai_family = AF_UNSPEC; + r = getaddrinfo(hostname, NULL, &hints, &info); + if (r != 0) + return NULL; + if (info->ai_canonname == NULL) { + freeaddrinfo(info); + return NULL; + } + + /* Caller frees this string. */ + result = strdup (info->ai_canonname); + freeaddrinfo(info); + return result; +} diff --git a/src/util.h b/src/util.h index 0748cbf..f85a61e 100644 --- a/src/util.h +++ b/src/util.h @@ -161,4 +161,6 @@ static inline int getuid (void) { return 0; } static inline int getgid (void) { return 0; } #endif +char *virGetHostname(void); + #endif /* __VIR_UTIL_H__ */ diff --git a/src/xen_unified.c b/src/xen_unified.c index a60bc79..dbda3b5 100644 --- a/src/xen_unified.c +++ b/src/xen_unified.c @@ -447,20 +447,15 @@ xenUnifiedGetVersion (virConnectPtr conn, unsigned long *hvVer) static char * xenUnifiedGetHostname (virConnectPtr conn) { - int r; - char hostname [HOST_NAME_MAX+1], *str; + char *result; - r = gethostname (hostname, HOST_NAME_MAX+1); - if (r == -1) { - xenUnifiedError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror(errno)); - return NULL; - } - str = strdup (hostname); - if (str == NULL) { + result = virGetHostname(); + if (result == NULL) { xenUnifiedError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror(errno)); return NULL; } - return str; + /* Caller frees this string. */ + return result; } static int -- 1.6.0.4