2:55 p.m.
On 06/24/2016 08:43 AM, John Ferlan wrote:
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.
This is just a re-hash of the missing VIR_FREE(ipStr) you found earlier.
I think all of these were caused by me messing up merge conflict
resolution when I re-ordered the patches - this code is actually
eliminate a patch or two later and replaced with a new function that (I
think - going to recheck) is correct.
> 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....
Yeah. My guess is that he was planning to output debug info to the log,
then never got around to it. Since the new functions that will replace
this are logging stuff, I just removed them.
> }
> }
> }
>
> /* 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
Yep. I'll do that.
> 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
And that.
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);
>