Since 98b9acf5aa02551dd37d0209339aba2e22e4004a This was a false positive where Coverity was complaining that the remoteDeserializeTypedParameters() could allocate 'params', but none of the callers could return the allocated memory back to their caller since on input the param was passed by value. Additionally, the flow of the code was that if params was NULL on entry, then each function would return 'nparams' as the number of params entries the caller would need to allocate in order to call the function again with 'nparams' and 'params' being set. By the time the deserialize routine was called params would have something. For other callers where the 'params' was passed by reference as NULL since it's expected that the deserialize allocates the memory and then have that passed back to the original caller to dispose there was no Coverity issue. As it turns out Coverity didn't quite seem to understand the relationship between 'nparams' and 'params'; however, if the !userAllocated path of the deserialize code compared against limit in any manner, then the Coverity error went away which was quite strange, but useful. As it turns out one code path remoteDomainGetJobStats had a comparison against 'limit' while another remoteConnectGetAllDomainStats did not assuming that limit would be checked. So I refactored the code a bit to cause the limit check to occur in deserialize for both conditions and then only made the check of current returned size against the incoming *nparams fail the non allocation case. This means the job code doesn't need to check the limit any more, while the stats code now does check the limit. Additionally, to help perhaps decipher which of the various callers to the deserialize code caused the failure - I used a #define to pass the __FUNCNAME__ of the caller along so that error messages could have something like: error: remoteConnectGetAllDomainStats: too many parameters '2' for nparams '0' error: Reconnected to the hypervisor (it's a contrived error just to show the funcname in the error) Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8221683..4a1b04b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -72,6 +72,11 @@ VIR_LOG_INIT("remote.remote_driver"); # define HYPER_TO_ULONG(_to, _from) (_to) = (_from) #endif +#define remoteDeserializeTypedParameters(ret_params_val, ret_params_len, \ + limit, params, nparams) \ + deserializeTypedParameters(__FUNCTION__, ret_params_val, ret_params_len, \ + limit, params, nparams) + static bool inside_daemon = false; static virDriverPtr remoteDriver = NULL; @@ -1743,21 +1748,30 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, /* Helper to deserialize typed parameters. */ static int -remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, - u_int ret_params_len, - int limit, - virTypedParameterPtr *params, - int *nparams) +deserializeTypedParameters(const char *funcname, + remote_typed_param *ret_params_val, + u_int ret_params_len, + int limit, + virTypedParameterPtr *params, + int *nparams) { size_t i = 0; int rv = -1; bool userAllocated = *params != NULL; + if (ret_params_len > limit) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for limit '%d'"), + funcname, ret_params_len, limit); + goto cleanup; + } + if (userAllocated) { /* Check the length of the returned list carefully. */ - if (ret_params_len > limit || ret_params_len > *nparams) { - virReportError(VIR_ERR_RPC, "%s", - _("returned number of parameters exceeds limit")); + if (ret_params_len > *nparams) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for nparams '%d'"), + funcname, ret_params_len, *nparams); goto cleanup; } } else { @@ -1774,8 +1788,8 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, if (virStrcpyStatic(param->field, ret_param->field) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Parameter %s too big for destination"), - ret_param->field); + _("%s: parameter %s too big for destination"), + funcname, ret_param->field); goto cleanup; } @@ -1811,8 +1825,8 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, goto cleanup; break; default: - virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"), - param->type); + virReportError(VIR_ERR_RPC, _("%s: unknown parameter type: %d"), + funcname, param->type); goto cleanup; } } @@ -6987,19 +7001,12 @@ remoteDomainGetJobStats(virDomainPtr domain, (xdrproc_t) xdr_remote_domain_get_job_stats_ret, (char *) &ret) == -1) goto done; - if (ret.params.params_len > REMOTE_DOMAIN_JOB_STATS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many job stats '%d' for limit '%d'"), - ret.params.params_len, - REMOTE_DOMAIN_JOB_STATS_MAX); - goto cleanup; - } - *type = ret.type; if (remoteDeserializeTypedParameters(ret.params.params_val, ret.params.params_len, - 0, params, nparams) < 0) + REMOTE_DOMAIN_JOB_STATS_MAX, + params, nparams) < 0) goto cleanup; rv = 0; -- 1.9.3

Coverity complains that because of how 'offset' is initialized to 0 (zero), the resulting math and comparison on rem is pointless. According to the origin commit id '3ec128989', the code is a replacement for gmtime(), but without the localtime() or GMT calculations - so just remove this code and add a comment indicating the removal Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virtime.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/util/virtime.c b/src/util/virtime.c index 9fefb67..acbec41 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -127,22 +127,16 @@ const unsigned short int __mon_yday[2][13] = { void virTimeFieldsThen(unsigned long long when, struct tm *fields) { /* This code is taken from GLibC under terms of LGPLv2+ */ + /* Remove the 'offset' or GMT manipulation since we don't care. See + * commit id '3ec12898' comments regarding localtime. + */ long int days, rem, y; const unsigned short int *ip; unsigned long long whenSecs = when / 1000ull; - unsigned int offset = 0; /* We hardcoded GMT */ days = whenSecs / SECS_PER_DAY; rem = whenSecs % SECS_PER_DAY; - rem += offset; - while (rem < 0) { - rem += SECS_PER_DAY; - --days; - } - while (rem >= SECS_PER_DAY) { - rem -= SECS_PER_DAY; - ++days; - } + fields->tm_hour = rem / SECS_PER_HOUR; rem %= SECS_PER_HOUR; fields->tm_min = rem / 60; -- 1.9.3

Coverity complains that the condition "size + 1 == 0" cannot happen. It's already been determined that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that offset+size didn't overflow. So just remove the check Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virstoragefile.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5db9184..1a02b18 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset) return BACKING_STORE_INVALID; - if (size + 1 == 0) - return BACKING_STORE_INVALID; if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; memcpy(*res, buf + offset, size); -- 1.9.3

On 09/15/2014 04:42 AM, Peter Krempa wrote: My previous change was : + if (size == UINT_MAX) return BACKING_STORE_INVALID; But it was pointed out by eblake: " Is this dead code? After all, we just checked that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that offset+size didn't overflow. " Would it be better to use the original change instead? Just trying to get past Coverity issue on this... BTW: The remainder of this w/r/t matching spec seems to be outside the scope of the Coverity DEADCODE, but if you have a patch I'm more than willing to look at it ;-) This code is shared with qcow1GetBackingStore and it's not like it's a recent change, so I'm hesitant to change it... John

Seems when commit id 'ea130e3b' added the checks to ensure each of the hard_limit, soft_limit, and swap_hard_limit wasn't set at VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - a copy/paste error of using the 'hard_limit' for each comparison was done. Adjust the code. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2a7d92..44db5d8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18088,9 +18088,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if ((def->mem.hard_limit && def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || (def->mem.soft_limit && - def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || + def->mem.soft_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || (def->mem.swap_hard_limit && - def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || + def->mem.swap_hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || def->mem.min_guarantee) { virBufferAddLit(buf, "<memtune>\n"); virBufferAdjustIndent(buf, 2); -- 1.9.3

Coverity complains that on the first pass through the for loop that 'params' cannot be true, thus the ternary setting to "&" cannot be done - which is I believe true - thus this really is a deadcode situation. It doesn't complain on the following comparison for "spice" - of course because we've set params = true now. I believe that's correct since we're putting together the line and the first "param" sets the "?" while the rest use "&". Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f80741c..a040e27 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9824,6 +9824,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) /* TLS Port */ if (tls_port) { + /* coverity[dead_error_condition] */ virBufferAsprintf(&buf, "%stls-port=%d", params ? "&" : "?", -- 1.9.3

Coverity complained that checking the return of virDomainCreate() was not consistent amongst the callers - so added the return check to the objecteventtest.c and adjust the virt-login-shell to compare < 0 rather than just non zero for the failure condition. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/objecteventtest.c | 3 ++- tools/virt-login-shell.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 919f559..052dbe5 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -359,7 +359,8 @@ testDomainStartStopEvent(const void *data) /* Test domain is started */ virDomainDestroy(dom); - virDomainCreate(dom); + if (virDomainCreate(dom) < 0) + goto cleanup; if (virEventRunDefaultImpl() < 0) goto cleanup; diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index be15a32..ceb271d 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -275,7 +275,7 @@ main(int argc, char **argv) if (!dom) goto cleanup; - if (!virDomainIsActive(dom) && virDomainCreate(dom)) { + if (!virDomainIsActive(dom) && virDomainCreate(dom) < 0) { virErrorPtr last_error; last_error = virGetLastError(); if (last_error->code != VIR_ERR_OPERATION_INVALID) { -- 1.9.3

Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len; /* If no active message, or fully transmitted, the no-op */ if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) @@ -489,22 +491,16 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) return -1; } + buf = mon->msg->txBuffer + mon->msg->txOffset; + len = mon->msg->txLength - mon->msg->txOffset; if (mon->msg->txFD == -1) - done = write(mon->fd, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset); + done = write(mon->fd, buf, len); else - done = qemuMonitorIOWriteWithFD(mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - mon->msg->txFD); + done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD); PROBE(QEMU_MONITOR_IO_WRITE, "mon=%p buf=%s len=%d ret=%d errno=%d", - mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - done, errno); + mon, buf, len, done, errno); if (mon->msg->txFD != -1) { PROBE(QEMU_MONITOR_IO_SEND_FD, -- 1.9.3

On 09/15/2014 04:09 AM, Peter Krempa wrote: Of course seen when formatting %d on the PROBE() I was reminded I had to change the type of the printf format ;-)... Also, I would be remiss if I didn't point out... struct _qemuMonitorMessage { int txFD; char *txBuffer; int txOffset; int txLength; ... John

On 09/15/2014 04:09 AM, Peter Krempa wrote: A build failure on a local jenkins server resulted in the following being pushed : commit c29cad67328bb8845c4396b85884211124ea1e2d Author: John Ferlan <jferlan(a)redhat.com&gt; Date: Mon Sep 15 11:37:20 2014 -0400 qemu: Fix build breaker on printf directive %zu for size_t not %lu diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 89446d7..3a32a4f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -499,7 +499,7 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD); PROBE(QEMU_MONITOR_IO_WRITE, - "mon=%p buf=%s len=%lu ret=%d errno=%d", + "mon=%p buf=%s len=%zu ret=%d errno=%d", mon, buf, len, done, errno); if (mon->msg->txFD != -1) {

On Sat, Sep 13, 2014 at 09:27:45AM -0400, John Ferlan wrote: Well, if sock_path cannot be NULL, then why not change it to: if (nfds > (1 + !!sock_path_ro)) { or something like that? Martin

On Mon, Sep 15, 2014 at 07:49:09AM -0400, John Ferlan wrote: Well, then that sa_assert() would make sense being there (where the dereference happens) or having a check only for that in some meaningful place. But I believe we're trying to solve a completely different problem. I was just stupid when I wrote the code :) But having it where you proposed it doesn't really make sense to me (or is not visible at first). Yes, and you can remove the "nfds &&" I believe (I have no idea why I used it there). ACK with this squashed in. Martin