[libvirt] [PATCH v2 0/4] Resolved CHECKED_RETURN errors found by Coverity
Rather then further muddy the waters of the previous patch series, this is a fresh patch series. I added one patch to this since the code was in the same general area as the virBufferTrim patch. Differences in v2 rpc: Updated the commit message to include why only putting out a VIR_WARN. The code did not change. The original patch: https://www.redhat.com/archives/libvir-list/2013-January/msg00103.html tools: Rework the patch to check the virBufferTrim() status return. Added virBufferError() status check after virBufferAddLit() to match the similar check after virBufferAddChar(). The original patch: https://www.redhat.com/archives/libvir-list/2013-January/msg00118.html xen: Resend of the v2 patch to keep in the same series: https://www.redhat.com/archives/libvir-list/2013-January/msg00473.html which was the replacement for: https://www.redhat.com/archives/libvir-list/2013-January/msg00114.html util: (new) The coverity error was NEGATIVE_RETURNS. The complaint being the potential to call memset() with a negative indent value. Since virBufferGetIndent() will check and fail on buf->error, I removed that check from virBufferAdd() and used the -1 return as the way to exit. John Ferlan (4): rpc: Check and message setsockopt() tools: Check return status on virBufferTrim() xen: Ignore return status for TCP_NODELAY util: Check for negative indent in virBufferAdd src/rpc/virnetsocket.c | 4 +++- src/util/virbuffer.c | 5 ++--- src/xen/xend_internal.c | 7 +++---- tools/virsh.c | 11 ++++++++--- 4 files changed, 16 insertions(+), 11 deletions(-) -- 1.7.11.7
Check status when attempting to set SO_REUSEADDR flag on outgoing connection On failure, VIR_WARN(), but continue to connect. This code path is on the sender side where the setting is just a hint and would only take effect if the sender is overflowed with TCP connections. Inability to set doesn't mean failure to establish a connection. --- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c31d383..aa523c0 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -472,7 +472,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; } - setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + } if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break; -- 1.7.11.7
On 01/15/2013 07:12 PM, John Ferlan wrote:
Check status when attempting to set SO_REUSEADDR flag on outgoing connection On failure, VIR_WARN(), but continue to connect. This code path is on the sender side where the setting is just a hint and would only take effect if the sender is overflowed with TCP connections. Inability to set doesn't mean failure to establish a connection. --- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c31d383..aa523c0 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -472,7 +472,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; }
- setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + }
if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break;
ACK && Pushed, Martin
--- tools/virsh.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index eadc519..669c9c9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -536,6 +536,8 @@ vshTreePrintInternal(vshControl *ctl, /* Finally print all children */ virBufferAddLit(indent, " "); + if (virBufferError(indent)) + goto cleanup; for (i = 0 ; i < num_devices ; i++) { const char *parent = (lookup)(i, true, opaque); @@ -545,15 +547,18 @@ vshTreePrintInternal(vshControl *ctl, false, indent) < 0) goto cleanup; } - virBufferTrim(indent, " ", -1); + if (virBufferTrim(indent, " ", -1) < 0) + goto cleanup; /* If there was no child device, and we're the last in * a list of devices, then print another blank line */ if (nextlastdev == -1 && devid == lastdev) vshPrint(ctl, "%s\n", virBufferCurrentContent(indent)); - if (!root) - virBufferTrim(indent, NULL, 2); + if (!root) { + if (virBufferTrim(indent, NULL, 2) < 0) + goto cleanup; + } ret = 0; cleanup: return ret; -- 1.7.11.7
On 01/15/2013 07:12 PM, John Ferlan wrote:
--- tools/virsh.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index eadc519..669c9c9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -536,6 +536,8 @@ vshTreePrintInternal(vshControl *ctl,
/* Finally print all children */ virBufferAddLit(indent, " "); + if (virBufferError(indent)) + goto cleanup; for (i = 0 ; i < num_devices ; i++) { const char *parent = (lookup)(i, true, opaque);
@@ -545,15 +547,18 @@ vshTreePrintInternal(vshControl *ctl, false, indent) < 0) goto cleanup; } - virBufferTrim(indent, " ", -1); + if (virBufferTrim(indent, " ", -1) < 0) + goto cleanup;
/* If there was no child device, and we're the last in * a list of devices, then print another blank line */ if (nextlastdev == -1 && devid == lastdev) vshPrint(ctl, "%s\n", virBufferCurrentContent(indent));
- if (!root) - virBufferTrim(indent, NULL, 2); + if (!root) { + if (virBufferTrim(indent, NULL, 2) < 0) + goto cleanup; + } ret = 0; cleanup: return ret;
ACK && Pushed, Martin
--- src/xen/xend_internal.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 959225c..038dd1e 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -89,11 +89,10 @@ do_connect(virConnectPtr xend) } /* - * try to desactivate slow-start + * try to deactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); - + ignore_value(setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start))); if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { VIR_FORCE_CLOSE(s); /* preserves errno */ -- 1.7.11.7
On 01/15/2013 07:12 PM, John Ferlan wrote:
--- src/xen/xend_internal.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 959225c..038dd1e 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -89,11 +89,10 @@ do_connect(virConnectPtr xend) }
/* - * try to desactivate slow-start + * try to deactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); - + ignore_value(setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start)));
if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { VIR_FORCE_CLOSE(s); /* preserves errno */
This one was already pushed by Eric in previous series, so I'm leaving it out. Martin
--- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 969dcbf..693e4b2 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -153,10 +153,9 @@ virBufferAdd(virBufferPtr buf, const char *str, int len) if (!str || !buf || (len == 0 && buf->indent == 0)) return; - if (buf->error) - return; - indent = virBufferGetIndent(buf, true); + if (indent < 0) + return; if (len < 0) len = strlen(str); -- 1.7.11.7
On 01/15/2013 07:12 PM, John Ferlan wrote:
--- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 969dcbf..693e4b2 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -153,10 +153,9 @@ virBufferAdd(virBufferPtr buf, const char *str, int len) if (!str || !buf || (len == 0 && buf->indent == 0)) return;
- if (buf->error) - return; - indent = virBufferGetIndent(buf, true); + if (indent < 0) + return;
if (len < 0) len = strlen(str);
ACK && Pushed with a little explanation taken from 0/4: Since virBufferGetIndent() will check and fail on buf->error, I removed that check from virBufferAdd() and used the -1 return as the way to exit. Hope that's OK, Martin
participants (2)
-
John Ferlan -
Martin Kletzander