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

On 05/03/2013 08:53 AM, Michal Privoznik wrote: Reviewed earlier. Part two of the review: Double OOM, but you also have a VIR_ALLOC, so this will be cleaned up in a later pass. Double OOM reporting; I'd just inline 'return -1' here instead of using goto. Yay - clean conversion! You could drop the error label and just inline the return -1. More instances where allowing a NULL source would simplify callers. Drop this double OOM. This one caught my eye, but it looks correct after all. Another case that could be simplified by taking a NULL source. This change is fine, but incomplete. You need to fix virDomainObjListGetInactiveNames a few lines later to quit reporting OOM when it sees data->oom set. double-oom is excusable because of VIR_ALLOC and again Changed from silent to noisy. Only caller appears to be remote_driver.c:remoteDomainBuildEventIOError, where failure is ignored (the event is basically discarded). Ouch - that code has a pre-existing bug - a NULL return here is eventually passed on to virDomainEventQueuePush, which queues up a NULL, and later servicing of the queue will do a NULL deref in virDomainEventDispatchMatchCallback. Looks like we need to fix that, and now's as good a time as any, possibly by fixing virDomainEventStateQueue to ignore a NULL event push, or possibly by fixing remote_driver.c. Changed from silent to noisy. Only caller appears to be qemu_process.c:qemuProcessHandleIOError, but at least this one properly discards the error without trying to queue up NULL. I'm wondering if these two particular functions should use the _QUIET variant, so we aren't polluting the thread-local error object in a case where the caller is fine discarding the error if it could not be created. silent->noisy. I did not audit this one for callers, but suspect it will be similar to IOError events: we probably have a bug in remote_driver.c for trying to queue NULL, and since all callers probably ignore an event object that could not be queued, using _QUIET here may be appropriate. Hmm, a bit more thinking: all events are asynchronous push, rather than synchronous replies to the client's request; which explains why we ignore OOM in creating an event - we aren't under obligation to send a reply. But I'm wondering if we should have code that tries to deliver an OOM event (of course, pre-created at the time the connection is first established, so that actually sending the OOM event doesn't involve any use of malloc), where hitting OOM and discarding any normal event then falls back to trying to fire the OOM event. Then again, OOM is such a rare corner case, and tends to be somewhat persistent as well; once we hit OOM trying to generate an error, we will probably also hit OOM on the next synchronous call from the client, so it won't be too much longer before they learn about it normally, without us having to worry about a special OOM event. So ignore this paragraph if you think I'm over-engineering things. Hmm, we weren't very consistent on which events were already noisy on creation failure. I guess that argues that using _QUIET variants is overkill, because we already have cases where an OOM will set the thread-local error object even if it event is discarded. Technically, *wwpn is already NULL if we get here, so you can drop this second VIR_FREE. double OOM here, but okay because VIR_ALLOC also uses the label. Again, VIR_ALLOC cleanup will cover this case of double oom Hmm, this code was missing oom reporting; but we'll clean that up on the VIR_ALLOC pass. Interesting. But looks correct. The majority of this patch looks reasonable. I already split capabilities.c into a separate review. domain_event.c is the only remaining file out of this lot that might warrant a split into its own patch, if you want to tackle making all event clients deal sanely with consistent semantics on what happens if you hit OOM when trying to report an event (and again, I'm not sure if the sanest is to clear the thread-local error, leave it polluted, or switch to _QUIET variants since we know that all events are best-effort only). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org