On 08/18/2016 07:47 AM, Erik Skultety wrote:
Continuing with the effort to split output parsing and defining, these new functions return a logging object reference instead of defining the output. Eventually, these functions will replace the existing ones (virLogAddOutputTo*) which will then be dropped. Also, make the new functions non-static, so they can be introduced prior to usage and the compiler won't complain (will be switched back to static once the usage is sorted out by later patches).
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 4 ++ src/util/virlog.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 6 +++ 3 files changed, 108 insertions(+)
Alternatively you could define them using one of the ATTRIBUTE_* macros - IIRC it's ATTRIBUTE_UNUSED, but it might be something different. I remember seeing it once and using it, but cannot recall what my fingers typed! Thus it would be "static $struct ATTRIBUTE_UNUSED" for each of the new definitions that will be static and that no one is calling. Then later when something calls it, just remove the ATTRIBUTE_UNUSED Thus no need to modify libvirt_private.syms BTW: I was wondering how long it was going to be before the virLogOutputNew would be used... Personally I would move usage closer to definition, but that's not a big deal.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bceba7..39b0270 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1857,6 +1857,10 @@ virLogGetNbOutputs; virLogGetOutputs; virLogLock; virLogMessage; +virLogNewOutputToFile; +virLogNewOutputToJournald; +virLogNewOutputToStderr; +virLogNewOutputToSyslog; virLogOutputFree; virLogOutputListFree; virLogOutputNew;
These wouldn't be necessary with that ATTRIBUTE_UNUSED I believe...
diff --git a/src/util/virlog.c b/src/util/virlog.c index a74967b..c0b8b9a 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -780,6 +780,14 @@ virLogAddOutputToStderr(virLogPriority priority) }
+virLogOutputPtr
static virLogOutputPtr ATTRIBUTE_UNUSED (same for all 4)
+virLogNewOutputToStderr(virLogPriority priority) +{ + return virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL);
Should '2L' be 'stderr' instead? Magic numbers are always a bit tricky... hmm.. nevermind my earlier comment about not allowing NULL for parameter 2 in the prototype... Sigh - even though I read ahead I forgot this usage model.
+} + + static int virLogAddOutputToFile(virLogPriority priority, const char *file) @@ -799,6 +807,27 @@ virLogAddOutputToFile(virLogPriority priority, }
+virLogOutputPtr +virLogNewOutputToFile(virLogPriority priority, + const char *file) +{ + int fd; + virLogOutputPtr ret = NULL; + + fd = open(file, O_CREAT | O_APPEND | O_WRONLY, S_IRUSR | S_IWUSR); + if (fd < 0) + return NULL; + + if (!(ret = virLogOutputNew(virLogOutputToFd, virLogCloseFd, + (void *)(intptr_t)fd, + priority, VIR_LOG_TO_FILE, file))) { + VIR_LOG_CLOSE(fd); + return NULL; + } + return ret; +} + + #if HAVE_SYSLOG_H || USE_JOURNALD
/* Compat in case we build with journald, but no syslog */ @@ -886,6 +915,51 @@ virLogAddOutputToSyslog(virLogPriority priority, }
+virLogOutputPtr +virLogNewOutputToSyslog(virLogPriority priority, + const char *ident) +{ + virLogOutputPtr ret = NULL; + int at = -1; + + /* There are a couple of issues with syslog: + * 1) If we re-opened the connection by calling openlog now, it would change + * the message tag immediately which is not what we want, since we might be + * in the middle of parsing of a new set of outputs where anything still can + * go wrong and we would introduce an inconsistent state to the log. We're + * also not holding a lock on the logger if we tried to change the tag + * while other workers are actively logging. + * + * 2) Syslog keeps the open file descriptor private, so we can't just dup() + * it like we would do with files if an output already existed. + * + * If a syslog connection already exists changing the message tag has to be + * therefore special-cased and postponed until the very last moment. + */ + if ((at = virLogFindOutput(virLogOutputs, virLogNbOutputs, + VIR_LOG_TO_SYSLOG, NULL)) < 0) { + /* + * rather than copying @ident, syslog uses caller's reference instead + */ + VIR_FREE(current_ident); + if (VIR_STRDUP(current_ident, ident) < 0) + return NULL; + + openlog(current_ident, 0, 0); + } + + if (!(ret = virLogOutputNew(virLogOutputToSyslog, virLogCloseSyslog, + NULL, priority, VIR_LOG_TO_SYSLOG, ident))) { + if (at < 0) { + closelog(); + VIR_FREE(current_ident); + } + return NULL; + } + return ret; +} + + # if USE_JOURNALD # define IOVEC_SET(iov, data, size) \ do { \ @@ -1102,6 +1176,30 @@ static int virLogAddOutputToJournald(int priority) } return 0; } + + +virLogOutputPtr +virLogNewOutputToJournald(int priority) +{ + virLogOutputPtr ret = NULL; + + if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) + return NULL; + + if (virSetInherit(journalfd, false) < 0) { + VIR_LOG_CLOSE(journalfd); + return NULL; + } + + if (!(ret = virLogOutputNew(virLogOutputToJournald, + virLogCloseJournald, NULL,
Reminder, if you move patch 19 to earlier, then journalfd needs to be passed here.
+ priority, VIR_LOG_TO_JOURNALD, NULL))) { + VIR_LOG_CLOSE(journalfd); + return NULL; + } + + return ret; +} # endif /* USE_JOURNALD */
int virLogPriorityFromSyslog(int priority) diff --git a/src/util/virlog.h b/src/util/virlog.h index e0fe008..e3d5243 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -239,5 +239,11 @@ int virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, virLogDestination dest, const void *opaque); int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs); int virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters); +virLogOutputPtr virLogNewOutputToStderr(virLogPriority priority); +virLogOutputPtr virLogNewOutputToFile(virLogPriority priority, + const char *file); +virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, + const char *ident); +virLogOutputPtr virLogNewOutputToJournald(int priority);
I believe if you use the ATTRIBUTE_UNUSED is used, then these aren't necessary either. ACK (in principal) with the edits. John
#endif