[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 25/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/util/*



On 05/03/2013 04:53 PM, Michal Privoznik wrote:
> ---
>  src/util/virauth.c               |  17 +--
>  src/util/virauthconfig.c         |   8 +-
>  src/util/virbitmap.c             |   9 +-
>  src/util/vircgroup.c             |  36 ++---
>  src/util/vircommand.c            |  33 ++---
>  src/util/virconf.c               |  34 ++---
>  src/util/virdnsmasq.c            |  22 +--
>  src/util/virebtables.c           |  34 ++---
>  src/util/virebtables.h           |   2 +-
>  src/util/virerror.c              |  19 +--
>  src/util/virhash.c               |   5 +-
>  src/util/viridentity.c           |  15 +-
>  src/util/virinitctl.c            |   4 +-
>  src/util/viriptables.c           |   4 +-
>  src/util/virjson.c               |  18 ++-
>  src/util/virkeyfile.c            |  13 +-
>  src/util/virlockspace.c          |  25 ++--
>  src/util/virlog.c                |  25 ++--
>  src/util/virnetdevmacvlan.c      |  16 +--
>  src/util/virnetdevtap.c          |  11 +-
>  src/util/virnetdevvportprofile.c |   4 +-
>  src/util/virobject.c             |  16 ++-
>  src/util/virpci.c                |  14 +-
>  src/util/virsexpr.c              |  37 ++---
>  src/util/virsocketaddr.c         |   9 +-
>  src/util/virstoragefile.c        |  18 +--
>  src/util/virstring.c             |  17 +--
>  src/util/virsysinfo.c            | 290 ++++++++++++++++-----------------------
>  src/util/virtypedparam.c         |  14 +-
>  src/util/viruri.c                |  58 ++++----
>  src/util/virutil.c               |  91 +++++-------
>  src/util/virxml.c                |   5 +-
>  32 files changed, 356 insertions(+), 567 deletions(-)
> 

> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index c81555a..cf6a099 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -37,6 +37,8 @@
>  #include "count-one-bits.h"
>  #include "virstring.h"
>  
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
>  struct _virBitmap {
>      size_t max_bit;
>      size_t map_len;
> @@ -226,8 +228,11 @@ char *virBitmapFormat(virBitmapPtr bitmap)
>          return NULL;
>  
>      cur = virBitmapNextSetBit(bitmap, -1);
> -    if (cur < 0)
> -        return strdup("");
> +    if (cur < 0) {
> +        char *ret;
> +        ignore_value(VIR_STRDUP(ret, ""));

VIR_STRDUP_QUIET (and drop the #define), otherwise the function will
report an error on OOM in VIR_STRDUP and it won't report it on OOM in the
virBuffer code. (unless you plan to convert those as well)

> +        return ret;
> +    }
>  
>      start = prev = cur;
>      while (prev >= 0) {
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 473d2fc..b3845ea 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -116,19 +116,14 @@ static int virCgroupCopyMounts(virCgroupPtr group,
>          if (!parent->controllers[i].mountPoint)
>              continue;
>  
> -        group->controllers[i].mountPoint =
> -            strdup(parent->controllers[i].mountPoint);
> -
> -        if (!group->controllers[i].mountPoint)
> +        if (VIR_STRDUP(group->controllers[i].mountPoint,
> +                       parent->controllers[i].mountPoint) < 0)

VIR_STRDUP_QUIET

>              return -ENOMEM;
>  
> -        if (parent->controllers[i].linkPoint) {
> -            group->controllers[i].linkPoint =
> -                strdup(parent->controllers[i].linkPoint);
> -
> -            if (!group->controllers[i].linkPoint)
> -                return -ENOMEM;
> -        }
> +        if (parent->controllers[i].linkPoint &&
> +            VIR_STRDUP(group->controllers[i].linkPoint,
> +                       parent->controllers[i].linkPoint) < 0)

here too

> +            return -ENOMEM;
>      }
>      return 0;
>  }
> @@ -177,8 +172,8 @@ static int virCgroupDetectMounts(virCgroupPtr group)
>                      struct stat sb;
>                      char *tmp2;
>  
> -                    if (!(group->controllers[i].mountPoint = strdup(entry.mnt_dir)))
> -                        goto no_memory;
> +                    if (VIR_STRDUP(group->controllers[i].mountPoint, entry.mnt_dir) < 0)
> +                        goto error;

VIR_STRDUP_QUIET and goto no_memory;

no_memory doesn't report an error here, it sets errno.

>  
>                      tmp2 = strrchr(entry.mnt_dir, '/');
>                      if (!tmp2) {
> @@ -239,7 +234,7 @@ static int virCgroupCopyPlacement(virCgroupPtr group,
>              continue;
>  
>          if (path[0] == '/') {
> -            if (!(group->controllers[i].placement = strdup(path)))
> +            if (VIR_STRDUP(group->controllers[i].placement, path) < 0)
>                  return -ENOMEM;
>          } else {
>              /*
> @@ -821,7 +816,7 @@ static int virCgroupNew(const char *path,
>      }
>  
>      if (path[0] == '/' || !parent) {
> -        if (!((*group)->path = strdup(path))) {
> +        if (VIR_STRDUP((*group)->path, path) < 0) {
>              rc = -ENOMEM;
>              goto err;
>          }

VIR_STRDUP_QUIET in both hunks above.

> @@ -1027,7 +1022,7 @@ static int virCgroupAddTaskStrController(virCgroupPtr group,
>      int rc = 0;
>      char *endp;
>  
> -    if (!(str = strdup(pidstr)))
> +    if (VIR_STRDUP(str, pidstr) < 0)
>          return -1;

VIR_STRDUP_QUIET
return -ENOMEM;

>  
>      cur = str;
> @@ -1253,7 +1248,7 @@ int virCgroupNewPartition(const char *path,
>  
>      if (STRNEQ(newpath, "/")) {
>          char *tmp;
> -        if (!(parentPath = strdup(newpath))) {
> +        if (VIR_STRDUP(parentPath, newpath) < 0) {

VIR_STRDUP_QUIET

>              rc = -ENOMEM;
>              goto cleanup;
>          }


> @@ -2543,12 +2538,11 @@ static char *virCgroupIdentifyRoot(virCgroupPtr group)
>          }
>

All this:

>          tmp[0] = '\0';
> -        ret = strdup(group->controllers[i].mountPoint);
> -        tmp[0] = '/';
> -        if (!ret) {
> -            virReportOOMError();
> +        if (VIR_STRDUP(ret, group->controllers[i].mountPoint) < 0) {
> +            tmp[0] = '/';
>              return NULL;
>          }
> +        tmp[0] = '/';

can be replaced by:
        ignore_value(VIR_STRNDUP(ret, group->controllers[i].mountPoint,
                                 tmp - group->controllers[i].mountPoint));

althought that would probably work better as a separate patch.

>          return ret;
>      }
>


> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index f6f27d9..eecda58 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -946,9 +946,8 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile)
>          return;
>  
>      VIR_FREE(cmd->pidfile);
> -    if (!(cmd->pidfile = strdup(pidfile))) {
> +    if (VIR_STRDUP(cmd->pidfile, pidfile) < 0)
>          cmd->has_error = ENOMEM;
> -    }
>  }
>  
>  
> @@ -1049,7 +1048,7 @@ virCommandSetSELinuxLabel(virCommandPtr cmd,
>  
>  #if defined(WITH_SECDRIVER_SELINUX)
>      VIR_FREE(cmd->seLinuxLabel);
> -    if (label && !(cmd->seLinuxLabel = strdup(label)))
> +    if (label && VIR_STRDUP(cmd->seLinuxLabel, label) < 0)
>          cmd->has_error = ENOMEM;
>  #endif
>      return;
> @@ -1074,7 +1073,7 @@ virCommandSetAppArmorProfile(virCommandPtr cmd,
>  
>  #if defined(WITH_SECDRIVER_APPARMOR)
>      VIR_FREE(cmd->appArmorProfile);
> -    if (profile && !(cmd->appArmorProfile = strdup(profile)))
> +    if (profile && VIR_STRDUP(cmd->appArmorProfile, profile) < 0)
>          cmd->has_error = ENOMEM;
>  #endif
>      return;
> @@ -1205,7 +1204,7 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str)
>      if (!cmd || cmd->has_error)
>          return;
>  
> -    if (!(env = strdup(str))) {
> +    if (VIR_STRDUP(env, str) < 0) {
>          cmd->has_error = ENOMEM;
>          return;
>      }
> @@ -1309,7 +1308,7 @@ virCommandAddArg(virCommandPtr cmd, const char *val)
>      if (!cmd || cmd->has_error)
>          return;
>  
> -    if (!(arg = strdup(val))) {
> +    if (VIR_STRDUP(arg, val) < 0) {
>          cmd->has_error = ENOMEM;
>          return;
>      }
> @@ -1350,11 +1349,11 @@ virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf)
>      }
>  
>      cmd->args[cmd->nargs] = virBufferContentAndReset(buf);
> -    if (!cmd->args[cmd->nargs])
> -        cmd->args[cmd->nargs] = strdup("");
>      if (!cmd->args[cmd->nargs]) {
> -        cmd->has_error = ENOMEM;
> -        return;
> +        if (VIR_STRDUP(cmd->args[cmd->nargs], "") < 0) {
> +            cmd->has_error = ENOMEM;
> +            return;
> +        }
>      }
>      cmd->nargs++;
>  }
> @@ -1440,8 +1439,9 @@ virCommandAddArgSet(virCommandPtr cmd, const char *const*vals)
>  
>      narg = 0;
>      while (vals[narg] != NULL) {
> -        char *arg = strdup(vals[narg++]);
> -        if (!arg) {
> +        char *arg;
> +
> +        if (VIR_STRDUP(arg, vals[narg++]) < 0) {
>              cmd->has_error = ENOMEM;
>              return;
>          }
> @@ -1481,8 +1481,7 @@ virCommandAddArgList(virCommandPtr cmd, ...)
>          char *arg = va_arg(list, char *);
>          if (!arg)
>              break;
> -        arg = strdup(arg);
> -        if (!arg) {
> +        if (VIR_STRDUP(arg, arg) < 0) {
>              cmd->has_error = ENOMEM;
>              va_end(list);
>              return;
> @@ -1511,8 +1510,7 @@ virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd)
>          cmd->has_error = -1;
>          VIR_DEBUG("cannot set directory twice");
>      } else {
> -        cmd->pwd = strdup(pwd);
> -        if (!cmd->pwd)
> +        if (VIR_STRDUP(cmd->pwd, pwd) < 0)
>              cmd->has_error = ENOMEM;
>      }
>  }
> @@ -1539,8 +1537,7 @@ virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf)
>          return;
>      }
>  
> -    cmd->inbuf = strdup(inbuf);
> -    if (!cmd->inbuf)
> +    if (VIR_STRDUP(cmd->inbuf, inbuf) < 0)
>          cmd->has_error = ENOMEM;
>  }
>

VIR_STRDUP_QUIET for all of the above.
OOM Error for these is reported in virCommandRun.



> diff --git a/src/util/virhash.c b/src/util/virhash.c
> index 2fe8751..bb708fc 100644
> --- a/src/util/virhash.c
> +++ b/src/util/virhash.c
> @@ -85,7 +86,9 @@ static bool virHashStrEqual(const void *namea, const void *nameb)
>  
>  static void *virHashStrCopy(const void *name)
>  {
> -    return strdup(name);
> +    char *ret;
> +    ignore_value(VIR_STRDUP(ret, name));
> +    return ret;
>  }
> 

VIR_STRDUP_QUIET, as the error gets reported in virHashAddOrUpdateEntry.

>  static void virHashStrFree(void *name)

> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index 06a1356..88c3bcd 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -118,10 +118,10 @@ iptRulesNew(const char *table,
>      if (VIR_ALLOC(rules) < 0)
>          return NULL;
>  
> -    if (!(rules->table = strdup(table)))
> +    if (VIR_STRDUP(rules->table, table) < 0)
>          goto error;
>  
> -    if (!(rules->chain = strdup(chain)))
> +    if (VIR_STRDUP(rules->chain, chain) < 0)
>          goto error;
>  
>      return rules;

VIR_STRDUP_QUIET, unless you plan to adjust the comment for iptablesContextNew
and remove the extra OOMError from bridge_driver.c

> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 92138d3..66376c1 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -107,7 +107,7 @@ virJSONValuePtr virJSONValueNewString(const char *data)
>          return NULL;
>  
>      val->type = VIR_JSON_TYPE_STRING;
> -    if (!(val->data.string = strdup(data))) {
> +    if (VIR_STRDUP(val->data.string, data) < 0) {
>          VIR_FREE(val);
>          return NULL;
>      }
> @@ -126,7 +126,7 @@ virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length)
>          return NULL;
>  
>      val->type = VIR_JSON_TYPE_STRING;
> -    if (!(val->data.string = strndup(data, length))) {
> +    if (VIR_STRNDUP(val->data.string, data, length) < 0) {
>          VIR_FREE(val);
>          return NULL;
>      }
> @@ -142,7 +142,7 @@ static virJSONValuePtr virJSONValueNewNumber(const char *data)
>          return NULL;
>  
>      val->type = VIR_JSON_TYPE_NUMBER;
> -    if (!(val->data.number = strdup(data))) {
> +    if (VIR_STRDUP(val->data.number, data) < 0) {
>          VIR_FREE(val);
>          return NULL;
>      }
> @@ -269,7 +269,7 @@ int virJSONValueObjectAppend(virJSONValuePtr object, const char *key, virJSONVal
>      if (virJSONValueObjectHasKey(object, key))
>          return -1;
>  
> -    if (!(newkey = strdup(key)))
> +    if (VIR_STRDUP(newkey, key) < 0)
>          return -1;
>  
>      if (VIR_REALLOC_N(object->data.object.pairs,
> @@ -751,10 +751,10 @@ static int virJSONParserHandleNumber(void *ctx,
>                                       yajl_size_t l)
>  {
>      virJSONParserPtr parser = ctx;
> -    char *str = strndup(s, l);
> +    char *str;
>      virJSONValuePtr value;
>  
> -    if (!str)
> +    if (VIR_STRNDUP(str, s, l) < 0)
>          return -1;
>      value = virJSONValueNewNumber(str);
>      VIR_FREE(str);
> @@ -808,8 +808,7 @@ static int virJSONParserHandleMapKey(void *ctx,
>      state = &parser->state[parser->nstate-1];
>      if (state->key)
>          return 0;
> -    state->key = strndup((const char *)stringVal, stringLen);
> -    if (!state->key)
> +    if (VIR_STRNDUP(state->key, (const char *)stringVal, stringLen) < 0)
>          return 0;
>      return 1;
>  }

All these are expected by the callers to be quiet.

> @@ -1094,8 +1093,7 @@ char *virJSONValueToString(virJSONValuePtr object,
>          goto cleanup;
>      }
>  
> -    if (!(ret = strdup((const char *)str)))
> -        virReportOOMError();
> +    ignore_value(VIR_STRDUP(ret, (const char *)str));
>  
>  cleanup:
>      yajl_gen_free(g);


> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index eee9ddc..2ed0b10 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -557,8 +557,7 @@ virLogDefineFilter(const char *match,
>          }
>      }
>  
> -    mdup = strdup(match);
> -    if (mdup == NULL) {
> +    if (VIR_STRDUP(mdup, match) < 0) {
>          i = -1;
>          goto cleanup;
>      }
> @@ -664,13 +663,9 @@ virLogDefineOutput(virLogOutputFunc f,
>      if (f == NULL)
>          return -1;
>  
> -    if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) {
> -        if (name == NULL)
> -            return -1;
> -        ndup = strdup(name);
> -        if (ndup == NULL)
> -            return -1;
> -    }
> +    if ((dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) &&
> +        (!name || VIR_STRDUP(ndup, name) < 0))
> +        return -1;
>  
>      virLogLock();
>      if (VIR_REALLOC_N(virLogOutputs, virLogNbOutputs + 1)) {
> @@ -1047,8 +1042,7 @@ virLogAddOutputToSyslog(virLogPriority priority,
>       * ident needs to be kept around on Solaris
>       */
>      VIR_FREE(current_ident);
> -    current_ident = strdup(ident);
> -    if (current_ident == NULL)
> +    if (VIR_STRDUP(current_ident, ident) < 0)
>          return -1;
>  
>      openlog(current_ident, 0, 0);
> @@ -1329,8 +1323,7 @@ virLogParseOutputs(const char *outputs)
>              if (str == cur)
>                  goto cleanup;
>  #if HAVE_SYSLOG_H
> -            name = strndup(str, cur - str);
> -            if (name == NULL)
> +            if (VIR_STRNDUP(name, str, cur - str) < 0)
>                  goto cleanup;
>              if (virLogAddOutputToSyslog(prio, name) == 0)
>                  count++;
> @@ -1346,8 +1339,7 @@ virLogParseOutputs(const char *outputs)
>                  cur++;
>              if (str == cur)
>                  goto cleanup;
> -            name = strndup(str, cur - str);
> -            if (name == NULL)
> +            if (VIR_STRNDUP(name, str, cur - str) < 0)
>                  goto cleanup;
>              if (virFileAbsPath(name, &abspath) < 0) {
>                  VIR_FREE(name);
> @@ -1424,8 +1416,7 @@ virLogParseFilters(const char *filters)
>              cur++;
>          if (str == cur)
>              goto cleanup;
> -        name = strndup(str, cur - str);
> -        if (name == NULL)
> +        if (VIR_STRNDUP(name, str, cur - str) < 0)
>              goto cleanup;
>          if (virLogDefineFilter(name, prio, flags) >= 0)
>              count++;

All the calls in this file should be quiet too.

> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 0c4fcbd..93bf798 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -764,14 +765,14 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
>      if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
>          if (VIR_ALLOC(calld) < 0)
>              goto memory_error;
> -        if ((calld->cr_ifname = strdup(ifname)) == NULL)
> -            goto memory_error;
> +        if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> +            goto error;
>          if (VIR_ALLOC(calld->virtPortProfile) < 0)
>              goto memory_error;
>          memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
>          virMacAddrSet(&calld->macaddress, macaddress);
> -        if ((calld->linkdev = strdup(linkdev)) == NULL)
> -            goto  memory_error;
> +        if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> +            goto  error;

double space       ^^

>          memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
>  
>          calld->vmOp = vmOp;

> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 93e37e4..72a5248 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -28,6 +28,7 @@
>  #include "viratomic.h"
>  #include "virerror.h"
>  #include "virlog.h"
> +#include "virstring.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
> @@ -37,7 +38,7 @@ struct _virClass {
>      virClassPtr parent;
>  
>      unsigned int magic;
> -    const char *name;
> +    char *name;
>      size_t objectSize;
>  
>      virObjectDisposeCallback dispose;
> @@ -131,21 +132,22 @@ virClassPtr virClassNew(virClassPtr parent,
>          return NULL;
>      }
>  
> -    if (VIR_ALLOC(klass) < 0)
> -        goto no_memory;
> +    if (VIR_ALLOC(klass) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
>  
>      klass->parent = parent;
> -    if (!(klass->name = strdup(name)))
> -        goto no_memory;
> +    if (VIR_STRDUP(klass->name, name) < 0)
> +        goto error;

Wouldn't it be nicer to do it with a temporary variable, so we can keep the const?

>      klass->magic = virAtomicIntInc(&magicCounter);
>      klass->objectSize = objectSize;
>      klass->dispose = dispose;
>  
>      return klass;
>  
> -no_memory:
> +error:
>      VIR_FREE(klass);
> -    virReportOOMError();
>      return NULL;
>  }
>



> diff --git a/src/util/virsexpr.c b/src/util/virsexpr.c
> index 23b6781..b17c0d4 100644
> --- a/src/util/virsexpr.c
> +++ b/src/util/virsexpr.c
> @@ -119,16 +119,10 @@ sexpr_string(const char *str, ssize_t len)
>      if (ret == NULL)
>          return ret;
>      ret->kind = SEXPR_VALUE;
> -    if (len > 0) {
> -        ret->u.value = strndup(str, len);
> -    } else {
> -        ret->u.value = strdup(str);
> -    }
>  
> -    if (ret->u.value == NULL) {
> +    if ((len > 0 && VIR_STRNDUP(ret->u.value, str, len) < 0) ||
> +        (len <= 0 && VIR_STRDUP(ret->u.value, str) < 0))
>          VIR_FREE(ret);
> -        return NULL;
> -    }
>  
>      return ret;
>  }

This function was quiet before, but it doesn't seem to be used anywhere.

> @@ -527,13 +511,10 @@ int sexpr_node_copy(const struct sexpr *sexpr, const char *node, char **dst)
>  {
>      const char *val = sexpr_node(sexpr, node);
>  
> -    if (val && *val) {
> -        *dst = strdup(val);
> -        if (!(*dst))
> -            return -1;
> -    } else {
> -        *dst = NULL;
> -    }
> +    if (val && *val)
> +        return VIR_STRDUP(*dst, val);
> +
> +    *dst = NULL;
>      return 0;
>  }
>

Callers report the OOM error for this function, but it seems it would be
better to leave it here and clean them up.

> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> index 2efe634..7fd2537 100644
> --- a/src/util/virsysinfo.c
> +++ b/src/util/virsysinfo.c
> @@ -144,32 +144,26 @@ virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret)
>      cur = strchr(cur, ':') + 1;
>      eol = strchr(cur, '\n');
>      virSkipSpaces(&cur);
> -    if (eol &&
> -       ((ret->system_family = strndup(cur, eol - cur)) == NULL))
> -         goto no_memory;
> +    if (eol && VIR_STRNDUP(ret->system_family, cur, eol - cur) < 0)
> +        return -1;
>  
>      if ((cur = strstr(base, "model")) != NULL) {
>          cur = strchr(cur, ':') + 1;
>          eol = strchr(cur, '\n');
>          virSkipSpaces(&cur);
> -        if (eol && ((ret->system_serial = strndup(cur, eol - cur))
> -                                                           == NULL))
> -            goto no_memory;
> +        if (eol && VIR_STRNDUP(ret->system_serial, cur, eol - cur) < 0)
> +            return -1;
>      }
>  
>      if ((cur = strstr(base, "machine")) != NULL) {
>          cur = strchr(cur, ':') + 1;
>          eol = strchr(cur, '\n');
>          virSkipSpaces(&cur);
> -        if (eol && ((ret->system_version = strndup(cur, eol - cur))
> -                                                            == NULL))
> -            goto no_memory;
> +        if (eol && VIR_STRNDUP(ret->system_version, cur, eol - cur) < 0)
> +            return -1;
>      }
>  
>      return 0;
> -
> -no_memory:
> -    return -1;
>  }
>  
>  static int
> @@ -185,34 +179,31 @@ virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret)
>          cur = strchr(base, ':') + 1;
>  
>          if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) {
> -            goto no_memory;
> +            return -1;
>          }
>          processor = &ret->processor[ret->nprocessor - 1];
>  
>          virSkipSpaces(&cur);
> -        if (eol &&
> -            ((processor->processor_socket_destination = strndup
> -                                     (cur, eol - cur)) == NULL))
> -            goto no_memory;
> +        if (eol && VIR_STRNDUP(processor->processor_socket_destination,
> +                               cur, eol - cur) < 0)
> +            return -1;
>  
>          if ((cur = strstr(base, "cpu")) != NULL) {
>              cur = strchr(cur, ':') + 1;
>              eol = strchr(cur, '\n');
>              virSkipSpaces(&cur);
> -            if (eol &&
> -               ((processor->processor_type = strndup(cur, eol - cur))
> -                                                             == NULL))
> -                goto no_memory;
> +            if (eol && VIR_STRNDUP(processor->processor_type,
> +                                   cur, eol - cur) < 0)
> +                return -1;
>          }
>  
>          if ((cur = strstr(base, "revision")) != NULL) {
>              cur = strchr(cur, ':') + 1;
>              eol = strchr(cur, '\n');
>              virSkipSpaces(&cur);
> -            if (eol &&
> -               ((processor->processor_version = strndup(cur, eol - cur))
> -                                                                == NULL))
> -                goto no_memory;
> +            if (eol && VIR_STRNUDP(processor->processor_version,

typo                              ^^

> +                                   cur, eol - cur) < 0)
> +                return -1;
>          }
>  
>          base = cur;

You have replaced all occurences of goto no_memory with return -1, but you
left the no_memory label here.

> @@ -270,32 +261,26 @@ virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret)
>      cur = strchr(cur, ':') + 1;
>      eol = strchr(cur, '\n');
>      virSkipSpaces(&cur);
> -    if (eol &&
> -       ((ret->system_family = strndup(cur, eol - cur)) == NULL))
> -         goto no_memory;
> +    if (eol && VIR_STRNDUP(ret->system_family, cur, eol - cur) < 0)
> +        return -1;
>  
>      if ((cur = strstr(base, "model")) != NULL) {
>          cur = strchr(cur, ':') + 1;
>          eol = strchr(cur, '\n');
>          virSkipSpaces(&cur);
> -        if (eol && ((ret->system_serial = strndup(cur, eol - cur))
> -                                                           == NULL))
> -            goto no_memory;
> +        if (eol && VIR_STRNDUP(ret->system_serial, cur, eol - cur) < 0)
> +            return -1;
>      }
>  
>      if ((cur = strstr(base, "machine")) != NULL) {
>          cur = strchr(cur, ':') + 1;
>          eol = strchr(cur, '\n');
>          virSkipSpaces(&cur);
> -        if (eol && ((ret->system_version = strndup(cur, eol - cur))
> -                                                            == NULL))
> -            goto no_memory;
> +        if (eol && VIR_STRNDUP(ret->system_version, cur, eol - cur) < 0)
> +            return -1;
>      }
>  
>      return 0;
> -
> -no_memory:
> -    return -1;
>  }
>  
>  static int
> @@ -313,10 +298,8 @@ virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret)
>      eol = strchr(base, '\n');
>      cur = strchr(base, ':') + 1;
>      virSkipSpaces(&cur);
> -    if (eol &&
> -        ((processor_type = strndup(cur, eol - cur))
> -         == NULL))
> -        goto no_memory;
> +    if (eol && VIR_STRNDUP(processor_type, cur, eol - cur) < 0)
> +        goto error;
>      base = cur;
>  
>      while ((tmp_base = strstr(base, "processor")) != NULL) {
> @@ -325,19 +308,20 @@ virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret)
>          cur = strchr(base, ':') + 1;
>  
>          if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) {
> -            goto no_memory;
> +            virReportOOMError();

Unrelated, but missing.

> +            goto error;
>          }
>          processor = &ret->processor[ret->nprocessor - 1];
>  
>          virSkipSpaces(&cur);
>          if (eol &&
> -            ((processor->processor_socket_destination = strndup
> -                                     (cur, eol - cur)) == NULL))
> -            goto no_memory;
> +            VIR_STRNDUP(processor->processor_socket_destination,
> +                        cur, eol - cur) < 0)
> +            goto error;
>  
>          if (processor_type &&
> -            !(processor->processor_type = strdup(processor_type)))
> -            goto no_memory;
> +            VIR_STRDUP(processor->processor_type, processor_type) < 0)
> +            goto error;
>  
>          base = cur;
>      }
>  

virSysinfoRead for non-windows x86 seems to be the only one reporting OOM
errors, which should be deleted after you switch VIR_ALLOC to report errors
too and the other architectures might be missing some OOM errors, like arm in
the hunk above.


> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 982d4a3..d3d77b2 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -547,11 +547,8 @@ virFileResolveLinkHelper(const char *linkpath,
>          if (lstat(linkpath, &st) < 0)
>              return -1;
>  
> -        if (!S_ISLNK(st.st_mode)) {
> -            if (!(*resultpath = strdup(linkpath)))
> -                return -1;
> -            return 0;
> -        }
> +        if (!S_ISLNK(st.st_mode))
> +            return VIR_STRDUP(*resultpath, linkpath);
>      }
>  
>      *resultpath = canonicalize_file_name(linkpath);

This function should return -1 and set errno on error, not report an error.

> @@ -623,9 +620,11 @@ char *virFindFileInPath(const char *file)
>       * copy of that path, after validating that it is executable
>       */
>      if (IS_ABSOLUTE_FILE_NAME(file)) {
> -        if (virFileIsExecutable(file))
> -            return strdup(file);
> -        else
> +        if (virFileIsExecutable(file)) {
> +            char *ret;
> +            ignore_value(VIR_STRDUP(ret, file));

VIR_STRDUP_QUIET

> +            return ret;
> +        } else
>              return NULL;
>      }
>  
> @@ -641,7 +640,7 @@ char *virFindFileInPath(const char *file)
>      /* copy PATH env so we can tweak it */
>      path = getenv("PATH");
>  
> -    if (path == NULL || (path = strdup(path)) == NULL)
> +    if (!path|| VIR_STRDUP(path, path) < 0)

missing space; VIR_STRDUP_QUIET

>          return NULL;
>  
>      /* for each path segment, append the file to search for and test for
> @@ -1296,7 +1295,7 @@ virFileMakePathWithMode(const char *path,
>      int ret = -1;
>      char *tmp;
>  
> -    if ((tmp = strdup(path)) == NULL)
> +    if (VIR_STRDUP(tmp, path) < 0)
>          goto cleanup;

This one should be quiet and set errno.

>  
>      ret = virFileMakePathHelper(tmp, mode);
> @@ -1503,8 +1502,7 @@ int virFileAbsPath(const char *path, char **abspath)
>      char *buf;
>  
>      if (path[0] == '/') {
> -        if (!(*abspath = strdup(path)))
> -            return -1;
> +        return VIR_STRDUP(*abspath, path);

VIR_STRDUP_QUIET

>      } else {
>          buf = getcwd(NULL, 0);
>          if (buf == NULL)


> @@ -2810,7 +2782,8 @@ char *virFileFindMountPoint(const char *type)
>  
>      while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
>          if (STREQ(mb.mnt_type, type)) {
> -            ret = strdup(mb.mnt_dir);
> +            if (VIR_STRDUP(ret, mb.mnt_dir) < 0)

VIR_STRDUP_QUIET

> +                errno = ENOMEM;
>              goto cleanup;
>          }
>      }

ACK (but I only compiled it on amd64 linux)

Jan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]