On 05/03/2013 08:53 AM, Michal Privoznik wrote:
--- src/cpu/cpu_generic.c | 8 ++++---- src/cpu/cpu_map.c | 3 ++- src/cpu/cpu_powerpc.c | 13 ++++++------- src/cpu/cpu_x86.c | 10 +++++----- 4 files changed, 17 insertions(+), 17 deletions(-)
@@ -131,7 +131,7 @@ genericBaseline(virCPUDefPtr *cpus, }
if (VIR_ALLOC(cpu) < 0 || - !(cpu->model = strdup(cpus[0]->model)) || + VIR_STRDUP(cpu->model, cpus[0]->model) < 0 || VIR_ALLOC_N(features, cpus[0]->nfeatures) < 0) goto no_memory;
double-oom reporting, will be cleaned up later, so not an issue for this patch.
@@ -183,8 +183,8 @@ genericBaseline(virCPUDefPtr *cpus, if (!features[i].name) continue;
- if (!(cpu->features[j++].name = strdup(features[i].name))) - goto no_memory; + if (VIR_STRDUP(cpu->features[j++].name, features[i].name) < 0) + goto error;
Another case of side effects (j++) inside the macro call. I think we are safe relying on exactly-once expansion of VIR_STRDUP macros, but we ought to push a followup patch that documents (in both HACKING and virstring.h) that the exactly-once semantics are guaranteed for this particular macro as an exception to the more general rule that any ALL_CAPS name is a macro that might mishandle side effects. (For that matter, VIR_ALLOC and VIR_INSERT_ELEMENT also have the same property of exactly-once semantics, and could also use the same documentation guarantee, but VIR_APPEND_ELEMENT double-evaluates its 'count' parameter, unless we rework virInsertElementsN() to take 'at==-1' to mean use '*countptr' as its initial 'at' value.)
+++ b/src/cpu/cpu_map.c @@ -27,6 +27,7 @@ #include "cpu.h" #include "cpu_map.h" #include "configmake.h" +#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_CPU
@@ -149,7 +150,7 @@ cpuMapOverride(const char *path) { char *map;
- if (!(map = strdup(path))) + if (VIR_STRDUP(map, path) < 0)
silent->noisy, but all callers are limited to tests/*, which just fail the test anyway, so no change needed.
+++ b/src/cpu/cpu_powerpc.c @@ -333,9 +333,8 @@ ppcDecode(virCPUDefPtr cpu, goto cleanup; }
- if (!(cpu->model = strdup(model->name)) || - (model->vendor && !(cpu->vendor = strdup(model->vendor->name)))) { - virReportOOMError(); + if (VIR_STRDUP(cpu->model, model->name) < 0 || + (model->vendor && VIR_STRDUP(cpu->vendor, model->vendor->name) < 0)) {
Another case where not requiring ATTRIBUTE_NONNNULL(2) on VIR_STRDUP would be handy. I think I've convinced myself we should go that route. Maybe it would help if I post a patch showing what I'm thinking?
goto cleanup; }
@@ -449,11 +448,11 @@ ppcBaseline(virCPUDefPtr *cpus, }
if (VIR_ALLOC(cpu) < 0 || - !(cpu->model = strdup(model->name))) + VIR_STRDUP(cpu->model, model->name) < 0) goto no_memory;
double-oom, but will be cleaned up later
+++ b/src/cpu/cpu_x86.c @@ -449,13 +449,13 @@ x86DataToCPU(const union cpuData *data, const struct x86_vendor *vendor;
if (VIR_ALLOC(cpu) < 0 || - !(cpu->model = strdup(model->name)) || + VIR_STRDUP(cpu->model, model->name) < 0 || !(copy = x86DataCopy(data)) || !(modelData = x86DataCopy(model->data))) goto no_memory;
double-oom, but will be cleaned up later
if ((vendor = x86DataToVendor(copy, map)) && - !(cpu->vendor = strdup(vendor->name))) + VIR_STRDUP(cpu->vendor, vendor->name) < 0) goto no_memory;
x86DataSubtract(copy, modelData); @@ -766,9 +766,9 @@ x86ModelCopy(const struct x86_model *model) { struct x86_model *copy;
- if (VIR_ALLOC(copy) < 0 - || !(copy->name = strdup(model->name)) - || !(copy->data = x86DataCopy(model->data))) { + if (VIR_ALLOC(copy) < 0 || + VIR_STRDUP(copy->name, model->name) < 0 || + !(copy->data = x86DataCopy(model->data))) { x86ModelFree(copy); return NULL;
silent->reporting, but function is static, and all callers already did 'goto no_memory', so this is merely another double-oom that will be cleaned up in your later pass ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org