[libvirt] [PATCH v2 07/20] virlog: Introduce virLogNewOutputTo* as a replacement for virLogAddOutputTo*
John Ferlan
jferlan at redhat.com
Wed Sep 21 18:17:25 UTC 2016
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 at 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
>
More information about the libvir-list
mailing list