Since we VIR_ALLOC_N to *values, the VIR_FREE should be done likewise Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index a41f896..ee54072 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1016,7 +1016,7 @@ int virConfGetValueStringList(virConfPtr conf, return -1; if (cval->str && VIR_STRDUP((*values)[0], cval->str) < 0) { - VIR_FREE(values); + VIR_FREE(*values); return -1; } break; -- 2.5.5

Cannot assume virGetLastError returns non-NULL value - modify the code to fetch err and check if err && err->code Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/qemuhelptest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 8aac997..7c8b841 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -60,7 +60,9 @@ static int testHelpStrParsing(const void *data) if (virQEMUCapsParseHelpStr("QEMU", help, flags, &version, &is_kvm, &kvm_version, false, NULL) == -1) { - if (info->error && virGetLastError()->code == info->error) + virErrorPtr err = virGetLastError(); + + if (info->error && err && err->code == info->error) ret = 0; goto cleanup; } -- 2.5.5

The 'res' variable was only being initialized to NULL in the if (!state) path; however, that path never used res and evenutally res is assigned one of two results based on a pair of if then else if conditions. If for some reason neither of those paths was taken and the (!state) path wasn't take, then 'res' would be indeterminate. Found by Coverity, probably a false positive based on code paths, but better safe than sorry for the future. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/vsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 68f7785..9ac4f21 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2636,7 +2636,8 @@ vshReadlineParse(const char *text, int state) vshCommandToken tk; static const vshCmdDef *cmd; const vshCmdOptDef *opt; - char *tkdata, *optstr, *const_tkdata, *res; + char *tkdata, *optstr, *const_tkdata; + char *res = NULL; static char *ctext, *sanitized_text; static uint64_t const_opts_need_arg, const_opts_required, const_opts_seen; uint64_t opts_need_arg, opts_required, opts_seen; @@ -2656,7 +2657,6 @@ vshReadlineParse(const char *text, int state) tkdata = NULL; sanitized_text = NULL; optstr = NULL; - res = NULL; /* Sanitize/de-quote the autocomplete text */ tk = sanitizer.getNextArg(NULL, &sanitizer, &sanitized_text, false); -- 2.5.5

Rather than use strtok_r using an input "path" variable which may or may not be NULL and thus alter the results, use the virStringSplitCount API in order to parse the path. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/esx/esx_vi.c | 61 ++++++++++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a28ac7b..f151dc4 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1179,22 +1179,17 @@ esxVI_Context_LookupManagedObjects(esxVI_Context *ctx) int esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) { + char **tmp = NULL; + size_t idx, ntmp = 0; int result = -1; - char *tmp = NULL; - char *saveptr = NULL; - char *previousItem = NULL; - char *item = NULL; virBuffer buffer = VIR_BUFFER_INITIALIZER; esxVI_ManagedObjectReference *root = NULL; esxVI_Folder *folder = NULL; - if (VIR_STRDUP(tmp, path) < 0) + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) goto cleanup; - /* Lookup Datacenter */ - item = strtok_r(tmp, "/", &saveptr); - - if (!item) { + if (ntmp == 0) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' does not specify a datacenter"), path); goto cleanup; @@ -1202,11 +1197,12 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = ctx->service->rootFolder; - while (!ctx->datacenter && item) { + for (idx = 0; idx < ntmp && !ctx->datacenter; ++idx) { + esxVI_Folder_Free(&folder); - /* Try to lookup item as a folder */ - if (esxVI_LookupFolder(ctx, item, root, NULL, &folder, + /* Try to lookup entry as a folder */ + if (esxVI_LookupFolder(ctx, tmp[idx], root, NULL, &folder, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1219,8 +1215,9 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = folder->_reference; folder->_reference = NULL; } else { - /* Try to lookup item as a datacenter */ - if (esxVI_LookupDatacenter(ctx, item, root, NULL, &ctx->datacenter, + /* Try to lookup entry as a datacenter */ + if (esxVI_LookupDatacenter(ctx, tmp[idx], root, NULL, + &ctx->datacenter, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1230,10 +1227,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (virBufferUse(&buffer) > 0) virBufferAddChar(&buffer, '/'); - virBufferAdd(&buffer, item, -1); - - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + virBufferAdd(&buffer, tmp[idx], -1); } if (!ctx->datacenter) { @@ -1248,9 +1242,10 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) ctx->datacenterPath = virBufferContentAndReset(&buffer); /* Lookup (Cluster)ComputeResource */ - if (!item) { + if (!tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, - _("Path '%s' does not specify a compute resource"), path); + _("Path '%s' does not specify a compute resource"), + path); goto cleanup; } @@ -1259,11 +1254,11 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = ctx->datacenter->hostFolder; - while (!ctx->computeResource && item) { + for (; idx < ntmp && !ctx->computeResource; ++idx) { esxVI_Folder_Free(&folder); - /* Try to lookup item as a folder */ - if (esxVI_LookupFolder(ctx, item, root, NULL, &folder, + /* Try to lookup entry as a folder */ + if (esxVI_LookupFolder(ctx, tmp[idx], root, NULL, &folder, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1276,8 +1271,8 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = folder->_reference; folder->_reference = NULL; } else { - /* Try to lookup item as a compute resource */ - if (esxVI_LookupComputeResource(ctx, item, root, NULL, + /* Try to lookup entry as a compute resource */ + if (esxVI_LookupComputeResource(ctx, tmp[idx], root, NULL, &ctx->computeResource, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; @@ -1288,10 +1283,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (virBufferUse(&buffer) > 0) virBufferAddChar(&buffer, '/'); - virBufferAdd(&buffer, item, -1); - - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + virBufferAdd(&buffer, tmp[idx], -1); } if (!ctx->computeResource) { @@ -1315,24 +1307,23 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) /* Lookup HostSystem */ if (STREQ(ctx->computeResource->_reference->type, "ClusterComputeResource")) { - if (!item) { + if (!tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' does not specify a host system"), path); goto cleanup; } /* The path specified a cluster, it has to specify a host system too */ - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + idx++; } - if (item) { + if (tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' ends with an excess item"), path); goto cleanup; } - if (VIR_STRDUP(ctx->hostSystemName, previousItem) < 0) + if (VIR_STRDUP(ctx->hostSystemName, tmp[idx - 1]) < 0) goto cleanup; if (esxVI_LookupHostSystem(ctx, ctx->hostSystemName, @@ -1359,7 +1350,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) esxVI_ManagedObjectReference_Free(&root); } - VIR_FREE(tmp); + virStringFreeListCount(tmp, ntmp); esxVI_Folder_Free(&folder); return result; -- 2.5.5

On Mon, Jul 18, 2016 at 03:06:58PM -0400, John Ferlan wrote: These two are unused since commit fb06350, they can be just removed. The only caller is qemuProcessStop and I think it can also be deleted. For persistent domains, the domain definition (including aliases) gets freed a few screens later when we replace it with newDef. For transient domains, we free the definition along with the virDomainObj a few moments later, but nothing that cares about aliases (neither building qemu command line nor talking to it via the monitor) should be done in the meantime. Jan

Rather than rely on strtok_r calls, use virStringSplitCount in order to process the conf variable. Coverity complains because it doesn't know that the returned temp variable in openvzReadVPSConfigParam should be non-NULL. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/openvz/openvz_conf.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 99ce95c..6562fcc 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -200,7 +200,9 @@ openvzReadNetworkConf(virDomainDefPtr def, int ret; virDomainNetDefPtr net = NULL; char *temp = NULL; - char *token, *saveptr = NULL; + char **tmp = NULL; + size_t ntmp = 0; + size_t i; /*parse routing network configuration* * Sample from config: @@ -214,20 +216,23 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, " ", &saveptr); - while (token != NULL) { + if (!(tmp = virStringSplitCount(temp, " ", 0, &ntmp))) + goto error; + + for (i = 0; i < ntmp; i++) { if (VIR_ALLOC(net) < 0) goto error; net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; - if (virDomainNetAppendIPAddress(net, token, AF_UNSPEC, 0) < 0) + if (virDomainNetAppendIPAddress(net, tmp[i], AF_UNSPEC, 0) < 0) goto error; if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) goto error; - - token = strtok_r(NULL, " ", &saveptr); } + virStringFreeListCount(tmp, ntmp); + ntmp = 0; + tmp = NULL; } /*parse bridge devices*/ @@ -242,15 +247,17 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, ";", &saveptr); - while (token != NULL) { - /*add new device to list*/ + if (!(tmp = virStringSplitCount(temp, " ", 0, &ntmp))) + goto error; + + for (i = 0; i < ntmp; i++) { + /* add new device to list */ if (VIR_ALLOC(net) < 0) goto error; net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - char *p = token; + char *p = tmp[i]; char cpy_temp[32]; int len; @@ -313,21 +320,21 @@ openvzReadNetworkConf(virDomainDefPtr def, } } p = ++next; - } while (p < token + strlen(token)); + } while (p < tmp[i] + strlen(tmp[i])); if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) goto error; - - token = strtok_r(NULL, ";", &saveptr); } } VIR_FREE(temp); + virStringFreeListCount(tmp, ntmp); return 0; error: VIR_FREE(temp); + virStringFreeListCount(tmp, ntmp); virDomainNetDefFree(net); return -1; } -- 2.5.5