Re: [libvirt] [PATCH 20/28] lxc: move debug/error log when adding IP addresses to virNetDevIPAddrAdd

It makes more sense to have the logging at the lower level so other callers can share the goodness. While removing so much stuff from / touching so many lines in lxcContainerRenameAndEnableInterfaces() (which used to have this debug/error logging), label names were changed and it was updated to use the now-more-common method of initializing ret to -1 (failure), then setting to 0 right before the cleanup label. --- src/lxc/lxc_container.c | 60 +++++++++++++++++++++---------------------------- src/util/virnetdevip.c | 32 ++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 39 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index a5ced92..09ad8cb 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -484,33 +484,32 @@ lxcContainerGetNetDef(virDomainDefPtr vmDef, const char *devName) * * Returns 0 on success or nonzero in case of error */ -static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, - size_t nveths, - char **veths) +static int +lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, + size_t nveths, + char **veths) { - int rc = 0; + int ret = -1; size_t i, j; const char *newname; - char *toStr = NULL; - char *viaStr = NULL; virDomainNetDefPtr netDef; bool privNet = vmDef->features[VIR_DOMAIN_FEATURE_PRIVNET] == VIR_TRISTATE_SWITCH_ON; for (i = 0; i < nveths; i++) { if (!(netDef = lxcContainerGetNetDef(vmDef, veths[i]))) - return -1; + goto cleanup; newname = netDef->ifname_guest; if (!newname) { - rc = -1; - goto error_out; + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing device name for container-side veth")); + goto cleanup; } VIR_DEBUG("Renaming %s to %s", veths[i], newname); - rc = virNetDevSetName(veths[i], newname); - if (rc < 0) - goto error_out; + if (virNetDevSetName(veths[i], newname) < 0) + goto cleanup; for (j = 0; j < netDef->guestIP.nips; j++) { virNetDevIPAddrPtr ip = netDef->guestIP.ips[j]; @@ -519,30 +518,23 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, if ((prefix = virSocketAddrGetIPPrefix(&ip->address, NULL, ip->prefix)) < 0) { + ipStr = virSocketAddrFormat(&ip->address);

virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to determine prefix for IP address '%s'"), ipStr); - goto error_out; - } - - VIR_DEBUG("Adding IP address '%s/%d' to '%s'", - ipStr, prefix, newname); - if (virNetDevIPAddrAdd(newname, &ip->address, NULL, prefix) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to set IP address '%s' on %s"), - ipStr, newname); VIR_FREE(ipStr); - goto error_out; + goto cleanup; } - VIR_FREE(ipStr); + + if (virNetDevIPAddrAdd(newname, &ip->address, NULL, prefix) < 0) + goto cleanup; } if (netDef->guestIP.nips || netDef->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP) { VIR_DEBUG("Enabling %s", newname); - rc = virNetDevSetOnline(newname, true); - if (rc < 0) - goto error_out; + if (virNetDevSetOnline(newname, true) < 0) + goto cleanup; /* Set the routes */ for (j = 0; j < netDef->guestIP.nroutes; j++) { @@ -553,22 +545,20 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, virNetDevIPRouteGetPrefix(route), virNetDevIPRouteGetGateway(route), virNetDevIPRouteGetMetric(route)) < 0) { - goto error_out; + goto cleanup; } - VIR_FREE(toStr); - VIR_FREE(viaStr);

} } } /* enable lo device only if there were other net devices */ - if (veths || privNet) - rc = virNetDevSetOnline("lo", true); + if ((veths || privNet) && + virNetDevSetOnline("lo", true) < 0) + goto cleanup; - error_out: - VIR_FREE(toStr); - VIR_FREE(viaStr); - return rc; + ret = 0; + cleanup: + return ret; } diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 376d4ad..dad342f 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -173,6 +173,13 @@ virNetDevIPAddrAdd(const char *ifname, struct nl_msg *nlmsg = NULL; struct nlmsghdr *resp = NULL; unsigned int recvbuflen; + char *ipStr = NULL; + char *peerStr = NULL; + char *bcastStr = NULL; + + ipStr = virSocketAddrFormat(addr);

+ if (peer && VIR_SOCKET_ADDR_VALID(peer)) + peerStr = virSocketAddrFormat(peer); /* The caller needs to provide a correct address */ if (VIR_SOCKET_ADDR_FAMILY(addr) == AF_INET && @@ -181,28 +188,45 @@ virNetDevIPAddrAdd(const char *ifname, if (VIR_ALLOC(broadcast) < 0) return -1;

- if (virSocketAddrBroadcastByPrefix(addr, prefix, broadcast) < 0) + if (virSocketAddrBroadcastByPrefix(addr, prefix, broadcast) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to determine broadcast address for '%s/%d'"), + ipStr, prefix); goto cleanup; + } + bcastStr = virSocketAddrFormat(broadcast); } + VIR_DEBUG("Adding IP address %s/%d%s%s%s%s to %s", ipStr, prefix, + peerStr ? " peer " : "", peerStr ? peerStr : "", + bcastStr ? " bcast " : "", bcastStr ? bcastStr : "", + ifname); +

if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_NEWADDR, ifname, addr, prefix, broadcast, peer))) goto cleanup; - if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0, - NETLINK_ROUTE, 0) < 0) + if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, + 0, 0, NETLINK_ROUTE, 0) < 0) goto cleanup; if (virNetlinkGetErrorCode(resp, recvbuflen) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, - _("Error adding IP address to %s"), ifname); + _("Failed to add IP address %s/%d%s%s%s%s to %s"), + ipStr, prefix, + peerStr ? " peer " : "", peerStr ? peerStr : "", + bcastStr ? " bcast " : "", bcastStr ? bcastStr : "", + ifname);

goto cleanup; } ret = 0; cleanup: + VIR_FREE(ipStr); + VIR_FREE(peerStr); + VIR_FREE(bcastStr); nlmsg_free(nlmsg); VIR_FREE(resp); VIR_FREE(broadcast);