Re: [libvirt] [PATCH v4 09/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/util/*

On 05/20/2013 11:55 AM, Michal Privoznik wrote: I've got my work cut out for me! 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. 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. again, double-oom 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. 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. 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. VIR_STRDUP_QUIET QUIET QUIET QUIET QUIET QUIET QUIET QUIET QUIET 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. Not for this patch, but why are we building up argv manually instead of using virCommand? This use of QUIET is correct. Simplify: if (VIR_STRDUP_QUIET(to->message, from->message) < 0) and so on. Can lose the surrounding if. and so on. 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. silent->noisy; looks like it needs to use _QUIET because callers ignore some errors. silent->noisy; I think some callers ignore failure, so QUIET may be better. and again. silent->noisy, but this one is probably okay. silent->noisy, but probably okay silent->noisy; probably okay, but we should adjust callers at some point silent->noisy, but probably a good change. True for most of this file. 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. 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. silent->noisy, but probably good silent->noisy, but rest of function isn't consistently noisy As long as you are touching this, kill the double space. if (VIR_STRNDUP(ret->u.value, str, len > 0 ? len : strlen(str)) < 0) Might have a merge conflict with the semicolon cleanup. silent->noisy, but probably a good change. silent->noisy, but probably good and more silent->noisy; probably the whole file is impacted. Another silent->noisy, and here you were dealing with VIR_EXPAND_N. 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