On 05/20/2013 11:55 AM, Michal Privoznik wrote:
> ---
> 34 files changed, 357 insertions(+), 570 deletions(-)
I've got my work cut out for me!
> @@ -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, ""));
> + return ret;
Hmm, I've seen this three-line pattern (declare temp var, strdup "" into
it, use the var) in several patches now. I think it might help to have
a new function in virstring.h whose job in life is to return a malloc'd
copy of an empty string, as a one-liner, so that callers don't have to
mess with a temp var. And notice that it's slightly more efficient to
just zero-initialize a malloc'd array of 1, instead of going through
strdup machinery, when we know the output will be an empty string. Maybe:
/* Return a malloc'd empty string, or NULL after reporting OOM */
char *
virStringEmpty(void)
{
char *ret;
// assuming we fix VIR_ALLOC to report oom...
ignore_value(VIR_ALLOC(ret));
return ret;
}
then THIS code could use the shorter:
if (cur < 0)
return virStringEmpty();
But if you decide to go that route, it's probably worth a separate
cleanup pass, so this commit is not delayed.
> +++ b/src/util/vircgroup.c
> @@ -116,19 +116,13 @@ 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)
> return -ENOMEM;
double-oom, since this function was previously silent and callers
already expected to do their own error reporting. This whole file has
an unusual paradigm compared to most source files, it may be best to
split this file into a separate patch (so that you aren't holding up the
rest of src/util/*) and either use VIR_STRDUP_QUIET or start to tackle
the bigger issue of tracing through callers to behave better when leaf
functions report errors.
>
> - if (parent->controllers[i].linkPoint) {
> - group->controllers[i].linkPoint =
> - strdup(parent->controllers[i].linkPoint);
> -
> - if (!group->controllers[i].linkPoint)
> - return -ENOMEM;
> - }
> + if (VIR_STRDUP(group->controllers[i].linkPoint,
> + parent->controllers[i].linkPoint) < 0)
> + return -ENOMEM;
again, double-oom
> @@ -177,7 +171,7 @@ static int virCgroupDetectMounts(virCgroupPtr group)
> struct stat sb;
> char *tmp2;
>
> - if (!(group->controllers[i].mountPoint = strdup(entry.mnt_dir)))
> + if (VIR_STRDUP(group->controllers[i].mountPoint, entry.mnt_dir) < 0)
> goto no_memory;
no_memory label is redudant; VIR_STRDUP guarantees that 'errno ==
ENOMEM' if it returns -1, as do VIR_ALLOC and virAsprintf. (Hmm, maybe
we should enhance './configure --enable-test-oom' to specifically test
that; although it may be a surprising amount of work to get that to
happen). This is a silent->noisy change, and again an instance where
the cgroup callers are doing their own error reporting; and the
no_memory label is a bit awkward because it is not doing its own OOM
reporting. Just deleting the no_memory label, and using 'goto error'
will make the code a bit less confusing. And it reiterates my thought
that src/util/vircgroup.c is enough of an oddball to warrant being split
into its own patch.
So with that, I'll quit pointing out silent->noisy changes in this file,
and just point out other problems.
> @@ -1027,8 +1021,8 @@ static int virCgroupAddTaskStrController(virCgroupPtr group,
> int rc = 0;
> char *endp;
>
> - if (!(str = strdup(pidstr)))
> - return -1;
> + if (VIR_STRDUP(str, pidstr) < 0)
> + return -ENOMEM;
Yikes! Are we mixing -1 and -errno in the same function? That's an
independent bug, and probably better to isolate that fix into an
independent commit.
> +++ 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;
double-oom. More precisely, we DON'T want to report oom until
virCommandRun sees cmd->has_error==ENOMEM, so that callers have the
option of building up a virCommand but ditching it halfway through if
they encounter some other (more important) error.
Thus, this should use VIR_STRDUP_QUIET.
> @@ -1049,7 +1048,7 @@ virCommandSetSELinuxLabel(virCommandPtr cmd,
>
> #if defined(WITH_SECDRIVER_SELINUX)
> VIR_FREE(cmd->seLinuxLabel);
> - if (label && !(cmd->seLinuxLabel = strdup(label)))
> + if (VIR_STRDUP(cmd->seLinuxLabel, label) < 0)
> cmd->has_error = ENOMEM;
VIR_STRDUP_QUIET
> #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 (VIR_STRDUP(cmd->appArmorProfile, profile) < 0)
> cmd->has_error = ENOMEM;
QUIET
> #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;
QUIET
> 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;
QUIET
> 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;
QUIET
> + 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;
QUIET
> 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;
QUIET
> 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;
QUIET
> }
> }
> @@ -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;
QUIET
> +++ b/src/util/virebtables.c
> @@ -187,10 +189,10 @@ ebtRulesNew(const char *table,
> if (VIR_ALLOC(rules) < 0)
> return NULL;
>
> - if (!(rules->table = strdup(table)))
> + if (VIR_STRDUP(rules->table, table) < 0)
> goto error;
silent->noisy, caller was already doing its own reporting based on errno
value. Probably a good change, but it means we probably ought to
eventually touch up qemuStateInitialize to not duplicate error handling.
> @@ -237,36 +239,36 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...)
>
> #if HAVE_FIREWALLD
> if (firewall_cmd_path) {
> - if (!(argv[n++] = strdup(firewall_cmd_path)))
> + if (VIR_STRDUP(argv[n++], firewall_cmd_path) < 0)
> goto error;
Not for this patch, but why are we building up argv manually instead of
using virCommand?
> +++ b/src/util/virerror.c
> @@ -164,7 +164,8 @@ virErrorGenericFailure(virErrorPtr err)
> err->code = VIR_ERR_INTERNAL_ERROR;
> err->domain = VIR_FROM_NONE;
> err->level = VIR_ERR_ERROR;
> - err->message = strdup(_("An error occurred, but the cause is unknown"));
> + ignore_value(VIR_STRDUP_QUIET(err->message,
> + _("An error occurred, but the cause is unknown")));
> }
This use of QUIET is correct.
>
>
> @@ -184,13 +185,13 @@ virCopyError(virErrorPtr from,
> to->code = from->code;
> to->domain = from->domain;
> to->level = from->level;
> - if (from->message && !(to->message = strdup(from->message)))
> + if (from->message && VIR_STRDUP_QUIET(to->message, from->message) < 0)
Simplify:
if (VIR_STRDUP_QUIET(to->message, from->message) < 0)
> ret = -1;
> - if (from->str1 && !(to->str1 = strdup(from->str1)))
> + if (from->str1 && VIR_STRDUP_QUIET(to->str1, from->str1) < 0)
> ret = -1;
> - if (from->str2 && !(to->str2 = strdup(from->str2)))
> + if (from->str2 && VIR_STRDUP_QUIET(to->str2, from->str2) < 0)
> ret = -1;
> - if (from->str3 && !(to->str3 = strdup(from->str3)))
> + if (from->str3 && VIR_STRDUP_QUIET(to->str3, from->str3) < 0)
and so on.
> @@ -687,11 +688,11 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
> to->message = str;
> to->level = level;
> if (str1 != NULL)
> - to->str1 = strdup(str1);
> + ignore_value(VIR_STRDUP_QUIET(to->str1, str1));
Can lose the surrounding if.
> if (str2 != NULL)
> - to->str2 = strdup(str2);
> + ignore_value(VIR_STRDUP_QUIET(to->str2, str2));
> if (str3 != NULL)
> - to->str3 = strdup(str3);
> + ignore_value(VIR_STRDUP_QUIET(to->str3, str3));
and so on.
> +++ b/src/util/virfile.c
> @@ -1059,7 +1059,7 @@ virFileFindMountPoint(const char *type)
>
> while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
> if (STREQ(mb.mnt_type, type)) {
> - ret = strdup(mb.mnt_dir);
> + ignore_value(VIR_STRDUP(ret, mb.mnt_dir));
> goto cleanup;
silent->noisy. This function has a bug: it says it returns NULL with
errno set, but then it does:
if (!ret)
errno = ENOENT;
cleanup:
endmntent(f);
return ret;
and endmntent is allowed to clobber errno. But I think the intent is
that we DO want this function to be silent (meaning this should use
QUIET), because some callers ignore certain errors.
> @@ -1299,11 +1299,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) < 0 ? -1 : 0 ;
silent->noisy; looks like it needs to use _QUIET because callers ignore
some errors.
> @@ -1377,10 +1374,10 @@ virFindFileInPath(const char *file)
> * copy of that path, after validating that it is executable
> */
> if (IS_ABSOLUTE_FILE_NAME(file)) {
> + char *ret = NULL;
> if (virFileIsExecutable(file))
> - return strdup(file);
> - else
> - return NULL;
> + ignore_value(VIR_STRDUP(ret, file));
silent->noisy; I think some callers ignore failure, so QUIET may be better.
> @@ -1395,7 +1392,7 @@ virFindFileInPath(const char *file)
> /* copy PATH env so we can tweak it */
> path = getenv("PATH");
>
> - if (path == NULL || (path = strdup(path)) == NULL)
> + if (VIR_STRDUP(path, path) <= 0)
> return NULL;
and again.
> @@ -2047,8 +2044,10 @@ virFileMakePathWithMode(const char *path,
> int ret = -1;
> char *tmp;
>
> - if ((tmp = strdup(path)) == NULL)
> + if (VIR_STRDUP(tmp, path) < 0) {
> + errno = ENOMEM;
> goto cleanup;
> + }
silent->noisy, but this one is probably okay.
> @@ -2257,7 +2256,7 @@ virFileAbsPath(const char *path, char **abspath)
> char *buf;
>
> if (path[0] == '/') {
> - if (!(*abspath = strdup(path)))
> + if (VIR_STRDUP(*abspath, path) < 0)
> return -1;
silent->noisy, but probably okay
> +++ b/src/util/viriptables.c
> @@ -119,10 +119,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;
silent->noisy; probably okay, but we should adjust callers at some point
> +++ 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;
silent->noisy, but probably a good change. True for most of this file.
> +++ b/src/util/virlog.c
> @@ -557,8 +557,7 @@ virLogDefineFilter(const char *match,
> }
> }
>
> - mdup = strdup(match);
> - if (mdup == NULL) {
> + if (VIR_STRDUP_QUIET(mdup, match) < 0) {
> i = -1;
> goto cleanup;
> }
> @@ -574,6 +573,8 @@ virLogDefineFilter(const char *match,
> virLogNbFilters++;
> cleanup:
> virLogUnlock();
> + if (i < 0)
> + virReportOOMError();
silent->noisy; there are other failure paths where we are still silent,
but it's probably better to be noisy. I think you did this construct so
that teh VIR_ALLOC_N also converts from silent->noisy, where a future
OOM cleanup path would then switch to VIR_STRDUP instead of
VIR_STRDUP_QUIET, although I'd rather live with double-oom now than
forget to drop QUIET later.
> @@ -664,13 +665,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) &&
> + VIR_STRDUP(ndup, name) <= 0)
> + return -1;
Hmm; this means you are silent->noisy on OOM, but still silent if name
was NULL. It might be better to be consistently noisy, even if that
means you can't use the <= as planned.
> @@ -1047,8 +1044,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;
silent->noisy, but probably good
> @@ -1329,8 +1325,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;
silent->noisy, but rest of function isn't consistently noisy
> +++ 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;
As long as you are touching this, kill the double space.
> +++ 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))
if (VIR_STRNDUP(ret->u.value, str, len > 0 ? len : strlen(str)) < 0)
> +++ b/src/util/virstring.c
> @@ -109,6 +109,7 @@ char **virStringSplit(const char *string,
>
> no_memory:
> virReportOOMError();
> +error:
> for (i = 0 ; i < ntokens ; i++)
Might have a merge conflict with the semicolon cleanup.
> +++ b/src/util/virsysinfo.c
> @@ -145,32 +145,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;
>
silent->noisy, but probably a good change.
> @@ -186,43 +180,37 @@ 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;
silent->noisy, but probably good
> @@ -271,32 +259,26 @@ virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret)
> @@ -314,10 +296,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;
and more silent->noisy; probably the whole file is impacted.
> base = cur;
>
> while ((tmp_base = strstr(base, "processor")) != NULL) {
> @@ -326,19 +306,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();
> + goto error;
> }
Another silent->noisy, and here you were dealing with VIR_EXPAND_N.
> @@ -463,7 +440,8 @@ virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret)
> goto cleanup;
> }
> processor = &ret->processor[ret->nprocessor - 1];
> - processor->processor_manufacturer = strdup(manufacturer);
> + if (VIR_STRDUP(processor->processor_manufacturer, manufacturer) < 0)
> + goto cleanup;
silent->noisy, but probably good.
Everything else looked good. There were a few problems to fix up before
pushing, and maybe it's worth resubmitting vircgroup cleanups (or doing
an explicit followup), but I can live with giving:
ACK
I agree that we want this series in before we freeze.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list