[libvirt] [PATCH 0/2] Some libvirt_nss cleanups
*** SOME PSEUDO-RANDOM BLURB *** Martin Kletzander (2): util: Add virSocketAddrSetIPv[46]AddrNetOrder and use it nss: Make aligning look nicer src/libvirt_private.syms | 2 ++ src/nwfilter/nwfilter_dhcpsnoop.c | 4 +-- src/util/virsocketaddr.c | 45 +++++++++++++++++++++++++++++----- src/util/virsocketaddr.h | 2 ++ tests/nsstest.c | 7 +++--- tools/nss/libvirt_nss.c | 51 +++++++++++++++++++++++++-------------- 6 files changed, 81 insertions(+), 30 deletions(-) -- 2.7.3
This allows setting the address if you already have the address in network byte order. Some places were having the address in network order and calling ntohl() just so the original function can call htonl() again. Let's call the new one to make clear what's happening. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- src/util/virsocketaddr.c | 45 +++++++++++++++++++++++++++++++++------ src/util/virsocketaddr.h | 2 ++ tests/nsstest.c | 7 +++--- 5 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ff803f996ac3..cfa3835c4ac4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2168,7 +2168,9 @@ virSocketAddrParseIPv4; virSocketAddrParseIPv6; virSocketAddrPrefixToNetmask; virSocketAddrSetIPv4Addr; +virSocketAddrSetIPv4AddrNetOrder; virSocketAddrSetIPv6Addr; +virSocketAddrSetIPv6AddrNetOrder; virSocketAddrSetPort; # util/virstats.h diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index c671cd88184d..702abe18d122 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1025,10 +1025,10 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, memset(&ipl, 0, sizeof(ipl)); memcpy(&nwint, &pd->d_yiaddr, sizeof(nwint)); - virSocketAddrSetIPv4Addr(&ipl.ipAddress, ntohl(nwint)); + virSocketAddrSetIPv4AddrNetOrder(&ipl.ipAddress, nwint); memcpy(&nwint, &pd->d_siaddr, sizeof(nwint)); - virSocketAddrSetIPv4Addr(&ipl.ipServer, ntohl(nwint)); + virSocketAddrSetIPv4AddrNetOrder(&ipl.ipServer, nwint); if (leasetime == ~0) ipl.timeout = ~0; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index b44d12e65216..53a8af898ead 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -173,6 +173,22 @@ virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val) } /* + * virSocketAddrSetIPv4AddrNetOrder: + * @addr: the location to store the result + * @val: the 128bit integer in network byte order representing the IPv4 address + * + * Set the IPv4 address given an integer in network order. This function does not + * touch any previously set port. + */ +void +virSocketAddrSetIPv4AddrNetOrder(virSocketAddrPtr addr, uint32_t val) +{ + addr->data.stor.ss_family = AF_INET; + addr->data.inet4.sin_addr.s_addr = val; + addr->len = sizeof(struct sockaddr_in); +} + +/* * virSocketAddrSetIPv4Addr: * @addr: the location to store the result * @val: the 32bit integer in host byte order representing the IPv4 address @@ -183,9 +199,22 @@ virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val) void virSocketAddrSetIPv4Addr(virSocketAddrPtr addr, uint32_t val) { - addr->data.stor.ss_family = AF_INET; - addr->data.inet4.sin_addr.s_addr = htonl(val); - addr->len = sizeof(struct sockaddr_in); + virSocketAddrSetIPv4AddrNetOrder(addr, htonl(val)); +} + +/* + * virSocketAddrSetIPv6AddrNetOrder: + * @addr: the location to store the result + * @val: the 128bit integer in network byte order representing the IPv6 address + * + * Set the IPv6 address given an integer in network order. This function does not + * touch any previously set port. + */ +void virSocketAddrSetIPv6AddrNetOrder(virSocketAddrPtr addr, uint32_t val[4]) +{ + addr->data.stor.ss_family = AF_INET6; + memcpy(addr->data.inet6.sin6_addr.s6_addr, val, 4 * sizeof(*val)); + addr->len = sizeof(struct sockaddr_in6); } /* @@ -198,9 +227,13 @@ virSocketAddrSetIPv4Addr(virSocketAddrPtr addr, uint32_t val) */ void virSocketAddrSetIPv6Addr(virSocketAddrPtr addr, uint32_t val[4]) { - addr->data.stor.ss_family = AF_INET6; - memcpy(addr->data.inet6.sin6_addr.s6_addr, val, 4 * sizeof(*val)); - addr->len = sizeof(struct sockaddr_in6); + size_t i = 0; + uint32_t host_val[4]; + + for (i = 0; i < 4; i++) + host_val[i] = htonl(val[i]); + + virSocketAddrSetIPv6AddrNetOrder(addr, host_val); } /* diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 9a6e1ecfa08a..c7aaa613b910 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -84,7 +84,9 @@ int virSocketAddrParseIPv4(virSocketAddrPtr addr, int virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val); +void virSocketAddrSetIPv4AddrNetOrder(virSocketAddrPtr s, uint32_t addr); void virSocketAddrSetIPv4Addr(virSocketAddrPtr s, uint32_t addr); +void virSocketAddrSetIPv6AddrNetOrder(virSocketAddrPtr s, uint32_t addr[4]); void virSocketAddrSetIPv6Addr(virSocketAddrPtr s, uint32_t addr[4]); char *virSocketAddrFormat(const virSocketAddr *addr); diff --git a/tests/nsstest.c b/tests/nsstest.c index 340f313616c5..68f1c600f6ec 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -126,15 +126,14 @@ testGetHostByName(const void *opaque) while (*addrList) { virSocketAddr sa; char *ipAddr; + void *address = *addrList; memset(&sa, 0, sizeof(sa)); if (resolved.h_addrtype == AF_INET) { - /* For some reason, virSocketAddrSetIPv4Addr does htonl() conversion. - * But the data we already have is in network order. */ - virSocketAddrSetIPv4Addr(&sa, ntohl(*((uint32_t *) *addrList))); + virSocketAddrSetIPv4AddrNetOrder(&sa, *((uint32_t *) address)); } else { - virSocketAddrSetIPv6Addr(&sa, (uint32_t *) *addrList); + virSocketAddrSetIPv6AddrNetOrder(&sa, address); } if (!(ipAddr = virSocketAddrFormat(&sa))) { -- 2.7.3
On Fri, Mar 18, 2016 at 17:42:28 +0100, Martin Kletzander wrote:
This allows setting the address if you already have the address in network byte order. Some places were having the address in network order and calling ntohl() just so the original function can call htonl() again. Let's call the new one to make clear what's happening.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- src/util/virsocketaddr.c | 45 +++++++++++++++++++++++++++++++++------ src/util/virsocketaddr.h | 2 ++ tests/nsstest.c | 7 +++--- 5 files changed, 48 insertions(+), 12 deletions(-)
[...]
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index b44d12e65216..53a8af898ead 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -173,6 +173,22 @@ virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val) }
/* + * virSocketAddrSetIPv4AddrNetOrder: + * @addr: the location to store the result + * @val: the 128bit integer in network byte order representing the IPv4 address
32 bit, for IPv4
+ * + * Set the IPv4 address given an integer in network order. This function does not + * touch any previously set port.
[...]
@@ -198,9 +227,13 @@ virSocketAddrSetIPv4Addr(virSocketAddrPtr addr, uint32_t val) */ void virSocketAddrSetIPv6Addr(virSocketAddrPtr addr, uint32_t val[4]) { - addr->data.stor.ss_family = AF_INET6; - memcpy(addr->data.inet6.sin6_addr.s6_addr, val, 4 * sizeof(*val)); - addr->len = sizeof(struct sockaddr_in6); + size_t i = 0; + uint32_t host_val[4]; + + for (i = 0; i < 4; i++) + host_val[i] = htonl(val[i]);
So the docs for this function were incorrect. It was indeed setting it in the network order. You should mention this change in the commit message. [...]
diff --git a/tests/nsstest.c b/tests/nsstest.c index 340f313616c5..68f1c600f6ec 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -126,15 +126,14 @@ testGetHostByName(const void *opaque) while (*addrList) { virSocketAddr sa; char *ipAddr; + void *address = *addrList;
memset(&sa, 0, sizeof(sa));
if (resolved.h_addrtype == AF_INET) { - /* For some reason, virSocketAddrSetIPv4Addr does htonl() conversion. - * But the data we already have is in network order. */ - virSocketAddrSetIPv4Addr(&sa, ntohl(*((uint32_t *) *addrList))); + virSocketAddrSetIPv4AddrNetOrder(&sa, *((uint32_t *) address)); } else { - virSocketAddrSetIPv6Addr(&sa, (uint32_t *) *addrList); + virSocketAddrSetIPv6AddrNetOrder(&sa, address);
Okay, this was the only place that is using the function fortunately. ACK with the two requested touch-ups. Peter
Every aligning requires at least one cast and it's hard to read. Let's make a function that makes sure the pointer is moved according to the alignment and use that to move throughout the data buffer. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/nss/libvirt_nss.c | 51 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index ba3bb31b3569..c9909901786d 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -252,17 +252,31 @@ _nss_libvirt_gethostbyname2_r(const char *name, int af, struct hostent *result, errnop, herrnop, NULL, NULL); } +static inline void * +move_and_align(void *buf, size_t len, size_t *idx) +{ + char *buffer = buf; + size_t move = ALIGN(len); + + if (!idx) + return buffer + move; + + *idx += move; + + return buffer + *idx; +} + enum nss_status _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp, char **canonp) { enum nss_status ret = NSS_STATUS_UNAVAIL; - char *r_name, **r_aliases, *r_addr, **r_addr_list; + char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; leaseAddress *addr = NULL; size_t naddr, i; bool found = false; - size_t nameLen, need, idx; + size_t nameLen, need, idx = 0; int alen; int r; @@ -319,23 +333,27 @@ _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, /* First, append name */ r_name = buffer; memcpy(r_name, name, nameLen + 1); - idx = ALIGN(nameLen + 1); + + r_aliases = move_and_align(buffer, nameLen + 1, &idx); /* Second, create aliases array */ - r_aliases = (char **) buffer + idx; r_aliases[0] = NULL; - idx += sizeof(char*); /* Third, append address */ - r_addr = buffer + idx; - for (i = 0; i < naddr; i++) - memcpy(r_addr + i * ALIGN(alen), addr[i].addr, alen); - idx += naddr * ALIGN(alen); + r_addr = move_and_align(buffer, sizeof(char *), &idx); + r_addr_next = r_addr; + for (i = 0; i < naddr; i++) { + memcpy(r_addr_next, addr[i].addr, alen); + r_addr_next = move_and_align(buffer, alen, &idx); + } + r_addr_list = move_and_align(buffer, 0, &idx); + r_addr_next = r_addr; /* Third, append address pointer array */ - r_addr_list = (char **) buffer + idx; - for (i = 0; i < naddr; i++) - r_addr_list[i] = r_addr + i * ALIGN(alen); + for (i = 0; i < naddr; i++) { + r_addr_list[i] = r_addr_next; + r_addr_next = move_and_align(r_addr_next, alen, NULL); + } r_addr_list[i] = NULL; idx += (naddr + 1) * sizeof(char*); @@ -420,25 +438,22 @@ _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat, /* First, append name */ r_name = buffer; memcpy(r_name, name, nameLen + 1); - idx = ALIGN(nameLen + 1); - /* Second, append addresses */ - r_tuple_first = (struct gaih_addrtuple*) (buffer + idx); + r_tuple_first = move_and_align(buffer, nameLen + 1, &idx); for (i = 0; i < naddr; i++) { int family = addr[i].af; - r_tuple = (struct gaih_addrtuple*) (buffer + idx); + r_tuple = move_and_align(buffer, 0, &idx); if (i == naddr - 1) r_tuple->next = NULL; else - r_tuple->next = (struct gaih_addrtuple*) ((char*) r_tuple + ALIGN(sizeof(struct gaih_addrtuple))); + r_tuple->next = move_and_align(buffer, sizeof(struct gaih_addrtuple), &idx); r_tuple->name = r_name; r_tuple->family = family; r_tuple->scopeid = 0; memcpy(r_tuple->addr, addr[i].addr, FAMILY_ADDRESS_SIZE(family)); - idx += ALIGN(sizeof(struct gaih_addrtuple)); } /* At this point, idx == need */ -- 2.7.3
On Fri, Mar 18, 2016 at 17:42:29 +0100, Martin Kletzander wrote:
Every aligning requires at least one cast and it's hard to read. Let's make a function that makes sure the pointer is moved according to the alignment and use that to move throughout the data buffer.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/nss/libvirt_nss.c | 51 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 18 deletions(-)
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index ba3bb31b3569..c9909901786d 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -252,17 +252,31 @@ _nss_libvirt_gethostbyname2_r(const char *name, int af, struct hostent *result,
[...]
_nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp, char **canonp) { enum nss_status ret = NSS_STATUS_UNAVAIL; - char *r_name, **r_aliases, *r_addr, **r_addr_list; + char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; leaseAddress *addr = NULL; size_t naddr, i; bool found = false; - size_t nameLen, need, idx; + size_t nameLen, need, idx = 0;
You've initialized it in this function ...
int alen; int r;
[...]
@@ -420,25 +438,22 @@ _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat,
... but not in this function ...
/* First, append name */ r_name = buffer; memcpy(r_name, name, nameLen + 1); - idx = ALIGN(nameLen + 1); -
... and you remove the initialization here ...
/* Second, append addresses */ - r_tuple_first = (struct gaih_addrtuple*) (buffer + idx); + r_tuple_first = move_and_align(buffer, nameLen + 1, &idx);
... and use it here: nss/libvirt_nss.c: In function '_nss_libvirt_gethostbyname4_r': nss/libvirt_nss.c:264:10: error: 'idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] *idx += move;
for (i = 0; i < naddr; i++) { int family = addr[i].af;
Peter
participants (2)
-
Martin Kletzander -
Peter Krempa