Re: [PATCH v2 07/20] virlog: Introduce virLogNewOutputTo* as a replacement for virLogAddOutputTo*

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
> ---
>  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
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]