Re: [libvirt] [PATCH v2 03/37] Adapt to VIR_STRDUP in daemon/*

On 04/29/2013 07:50 AM, Michal Privoznik wrote: We should just nuke this error, and let VIR_STRDUP's OOM error suffice. This whole function could probably use a cleanup pass to use virReportOOMError() as appropriate. Yep, here's another case where we should nuke the virReportError(VIR_ERR_CONFIG_UNSUPPORTED) and rely on VIR_STRDUP's OOM error instead. Drop the virReportOOMError here; since VIR_STRDUP is documented to do the work for you. You can drop the no_memory label and just use the error label. Hmm. Here, the no_memory label does additional cleanup... ...and one of the paths into the no_memory label is via virAsprintf(), which has not yet been converted to report OOM. So for this function, you'll need to add a label (and later delete no_memory when we do a second pass over the code to scrub virAsprintf usage): no_memory: virReportOOMError(); cleanup: daemonConfigFree(data); return NULL; Drop the duplicate error. Change to goto error. Change both to goto error Don't report the error twice. _This_ instance might need to use VIR_STRDUP_QUIET and keep the VIR_ERROR() report; main() is early enough that virReportOOMError() might not go anywhere useful. Probably will never be triggered in practice, though, so I'd also be okay with regular VIR_STRDUP and dropping the VIR_ERROR. Same story. Here, I'd hoist the OOM error that occurs several lines later into this if branch (since it can't be reached on the else branch) rather than using ignore_value just to then check for NULL ourselves a few lines later. Which turns this into: if (privileged) { if (VIR_STRDUP(run_dir, LOCALSTATEDIR "/run/libvirt") < 0) goto cleanup; } else { run_dir = virGetUserRuntimeDirectory(); Drop the virReportOOMError() from mem_error(); at which point the label should be renamed to plain 'error'. Same story (in fact, true for much of this file, so I'll quit pointing it out). Ah, now here's where we have to choose to either report OOM twice on VIR_STRDUP failure until a second pass of patches cleans up VIR_ALLOC to report OOM, or where we temporarily have to make the code ugly so that VIR_ALLOC uses goto mem_error while VIR_STRDUP uses goto error. I guess I can live with either version, as long as we do get the VIR_ALLOC usage scrubbed before 1.0.6. Drop the error report. And again. And again. Here too. Here, goto cleanup. goto cleanup drop the duplicate error report Ouch. Pre-existing bug that we leave dom_dst->name as NULL on OOM. It means that we could segfault instead of gracefully returning an error to the user when we ran out of memory. We probably ought to clean that up first (in fact, it would be appropriate to even clean it up before 1.0.5 - I might tackle that if I have time later today). You should NOT have to wrap ignore_value() around every caller of VIR_STRDUP_QUIET. What that says to me is that the macro you write in patch 2/37 needs to inline the ignore_value() as part of the macro. Hmm. All of our make_nonnull_* functions share that bug. This one's another case of VIR_ALLOC_N and VIR_STRDUP both feeding into the same label; up to you whether to temporarily leave this one as double-reporting (until the VIR_ALLOC audit), or whether to re-arrange the label to have no_memory report OOM first instead of last, as well as add a new error: label for the VIR_STRDUP to jump to. It looks like we still have a long ways to go before this series is ready - I don't see any point in churning the code to add a silent VIR_STRDUP and then make a second pass that turns it to be noisy - it's simpler in the git history if we just start life with VIR_STRDUP as noisy and make the cleanups as we go as part of converting to use VIR_STRDUP. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org