[libvirt] [PATCH v3 04/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/cpu/*

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130506/9e3ab87d/attachment-0001.sig>