On 08/18/2016 07:47 AM, Erik Skultety wrote:
Now that we're in the critical section, syslog connection can be re-opened by issuing openlog, which is something that cannot be done beforehand, since syslog keeps its file descriptor private and changing the tag earlier might introduce a log inconsistency if something went wrong with preparing a new set of logging outputs in order to replace the existing one.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virlog.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c index c0b8b9a..61e71a3 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1794,16 +1794,35 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs) { + int ret = -1; + size_t i; + char *tmp = NULL; + if (virLogInitialize() < 0) return -1;
virLogLock(); virLogResetOutputs(); + + /* nothing can go wrong now (except for malloc) and we're also holding the + * lock so it's safe to call openlog and change the message tag */ + for (i = 0; i < noutputs; i++) { + if (outputs[i]->dest == VIR_LOG_TO_SYSLOG) {
Is this essentially open coded virLogFindOutput? Or is it that the ->name and current_ident check that would trip that up based on the weirdness from the previous patch? If you modify the Find to do that STREQ_NULLABLE check - I think then that function could be used...
+ if (VIR_STRDUP_QUIET(tmp, outputs[i]->name) < 0) + goto cleanup; + VIR_FREE(current_ident); + current_ident = tmp; + openlog(current_ident, 0, 0); + } + } + virLogOutputs = outputs; virLogNbOutputs = noutputs; - virLogUnlock();
- return virLogNbOutputs; + ret = virLogNbOutputs;
BTW: Still see no reason to not have ret be 0... From the earlier comment... Conditional ACK... curious about usage of virFindLogOutput... John
+ cleanup: + virLogUnlock(); + return ret; }