On 06/22/2016 01:37 PM, Laine Stump wrote:
> 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);
Coverity complains here that ipStr is overwritten (from 4 lines up -
char *ipStr = virSocketAddrFormat(&ip->address);)
It seems the first one is duplicitous.
> 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);
Hmm... Seems commit id 'a117652917e' forgot about 'em....
> }
> }
> }
>
> /* 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);
Note - no check if ipStr is NULL...
> + 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;
Coverity let me know this causes ipStr to be leaked. Probably similar
for peerStr too, but Coverity didn't note that one.
>
> - 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);
> +
NULLSTR() or perhaps EMPTYSTR()? For ipStr, peerStr, and bcastStr...
In any case, since ipStr could be NULL it needs the similar check
> 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);
Similar comment regarding ipStr, peerStr, and bcastStr usage
ACK with the adjustments,
John
> goto cleanup;
> }
>
> ret = 0;
> cleanup:
> + VIR_FREE(ipStr);
> + VIR_FREE(peerStr);
> + VIR_FREE(bcastStr);
> nlmsg_free(nlmsg);
> VIR_FREE(resp);
> VIR_FREE(broadcast);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list