Re: [libvirt] [PATCH 02/16] util: Combine __virExec and virExecWithHook

On 05/10/2011 02:07 PM, Cole Robinson wrote: > All callers were expecting argv logging, so the split is unneeded. Not quite true - virCommandRunAsync already does an independent VIR_DEBUG of argv/envp prior to calling virExecWithHook, so we are actually doing double-duty at the moment. But that can be a later cleanup. > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- > src/util/util.c | 86 ++++++++++++++++++++++-------------------------------- > 1 files changed, 35 insertions(+), 51 deletions(-) > > diff --git a/src/util/util.c b/src/util/util.c > index 1d0c2cc..f860392 100644 > --- a/src/util/util.c > + if ((argv_str = virArgvToString(argv)) == NULL) { > + virReportOOMError(); > + return -1; > + } > + > + if (envp) { > + if ((envp_str = virArgvToString(envp)) == NULL) { > + VIR_FREE(argv_str); > + virReportOOMError(); > + return -1; > + } > + VIR_DEBUG("%s %s", envp_str, argv_str); > + VIR_FREE(envp_str); > + } else { > + VIR_DEBUG0(argv_str); > + } > + VIR_FREE(argv_str); Pre-existing, but failure to log the argv/envp string shouldn't necessarily abort the attempt to run the command; see how command.c falls back to the simpler argv[0] when argv_str couldn't be computed. However, with regards to pure refactorization and code motion, I don't see any regressions, so: ACK with one nit: > > - if ((execret = __virExec(argv, NULL, NULL, > + if ((execret = virExecWithHook(argv, NULL, NULL, > &childpid, -1, &outfd, &errfd, > VIR_EXEC_NONE, hook, data, NULL)) < 0) { Reindent these lines to match the new column of ( in the change line above.