[libvirt] [PATCH v2 02/13] Import stripped down virtlockd code as basis of virtlogd

[Peter Krempa]

On Thu, Nov 12, 2015 at 17:18:59 +0000, Daniel Berrange wrote:
> Copy the virtlockd codebase across to form the initial virlogd
> code. Simple search & replace of s/lock/log/ and gut the remote
> protocol & dispatcher. This gives us a daemon that starts up
> and listens for connections, but does nothing with them.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---

[...]

> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> new file mode 100644
> index 0000000..184076c
> --- /dev/null
> +++ b/src/logging/log_daemon.c
> @@ -0,0 +1,1177 @@
> +/*
> + * log_daemon.c: log management daemon
> + *
> + * Copyright (C) 2006-2015 Red Hat, Inc.

Um 2006? Here and in every other header.

[...]

> +
> +struct _virLogDaemon {
> +    virMutex lock;
> +    virNetDaemonPtr dmn;
> +    virNetServerPtr srv;
> +};
> +
> +virLogDaemonPtr logDaemon = NULL;
> +
> +static bool execRestart;
> +
> +enum {
> +    VIR_LOG_DAEMON_ERR_NONE = 0,
> +    VIR_LOG_DAEMON_ERR_PIDFILE,
> +    VIR_LOG_DAEMON_ERR_RUNDIR,
> +    VIR_LOG_DAEMON_ERR_INIT,
> +    VIR_LOG_DAEMON_ERR_SIGNAL,
> +    VIR_LOG_DAEMON_ERR_PRIVS,
> +    VIR_LOG_DAEMON_ERR_NETWORK,
> +    VIR_LOG_DAEMON_ERR_CONFIG,
> +    VIR_LOG_DAEMON_ERR_HOOKS,
> +    VIR_LOG_DAEMON_ERR_REEXEC,
> +
> +    VIR_LOG_DAEMON_ERR_LAST
> +};
> +
> +VIR_ENUM_DECL(virDaemonErr)
> +VIR_ENUM_IMPL(virDaemonErr, VIR_LOG_DAEMON_ERR_LAST,
> +              "Initialization successful",
> +              "Unable to obtain pidfile",
> +              "Unable to create rundir",
> +              "Unable to initialize libvirt",

Will this need to call libvirt? Or should this be 'virtlogd'?

> +              "Unable to setup signal handlers",
> +              "Unable to drop privileges",
> +              "Unable to initialize network sockets",
> +              "Unable to load configuration file",
> +              "Unable to look for hook scripts",
> +              "Unable to re-execute daemon");
> +

[...]

> +
> +
> +static void
> +virLogDaemonErrorHandler(void *opaque ATTRIBUTE_UNUSED,
> +                         virErrorPtr err ATTRIBUTE_UNUSED)
> +{
> +    /* Don't do anything, since logging infrastructure already
> +     * took care of reporting the error */
> +}
> +
> +
> +/*
> + * Set up the logging environment
> + * By default if daemonized all errors go to the logfile libvirtd.log,
> + * but if verbose or error debugging is asked for then also output
> + * informational and debug messages. Default size if 64 kB.

The logging ring buffer isn't present any more.

> + */
> +static int
> +virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> +                         bool privileged,
> +                         bool verbose,
> +                         bool godaemon)
> +{
> +    virLogReset();
> +
> +    /*
> +     * Libvirtd's order of precedence is:

libvirtd?

[...]

> +
> +static void
> +virLogDaemonUsage(const char *argv0, bool privileged)
> +{
> +    fprintf(stderr,
> +            _("\n"
> +              "Usage:\n"
> +              "  %s [options]\n"
> +              "\n"
> +              "Options:\n"
> +              "  -h | --help            Display program help:\n"
> +              "  -v | --verbose         Verbose messages.\n"
> +              "  -d | --daemon          Run as a daemon & write PID file.\n"
> +              "  -t | --timeout <secs>  Exit after timeout period.\n"
> +              "  -f | --config <file>   Configuration file.\n"
> +              "  -V | --version         Display version information.\n"
> +              "  -p | --pid-file <file> Change name of PID file.\n"
> +              "\n"
> +              "libvirt lock management daemon:\n"), argv0);

Log management.

[...]

> +#define MAX_LISTEN 5

This macro isn't used in this patch.

> +int main(int argc, char **argv) {


[...]


> diff --git a/src/logging/log_daemon_config.c b/src/logging/log_daemon_config.c
> new file mode 100644
> index 0000000..98d4c89
> --- /dev/null
> +++ b/src/logging/log_daemon_config.c
> @@ -0,0 +1,203 @@

[...]

> +/* Read the config file if it exists.
> + * Only used in the remote case, hence the name.

name?

> + */
> +int
> +virLogDaemonConfigLoadFile(virLogDaemonConfigPtr data,
> +                           const char *filename,
> +                           bool allow_missing)
> +{
> +    virConfPtr conf;
> +    int ret;
> +
> +    if (allow_missing &&
> +        access(filename, R_OK) == -1 &&
> +        errno == ENOENT)
> +        return 0;
> +
> +    conf = virConfReadFile(filename, 0);
> +    if (!conf)
> +        return -1;
> +
> +    ret = virLogDaemonConfigLoadOptions(data, filename, conf);
> +    virConfFree(conf);
> +    return ret;
> +}

[...]

> diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> new file mode 100644
> index 0000000..9b8fa41
> --- /dev/null
> +++ b/src/logging/log_protocol.x
> @@ -0,0 +1,22 @@
> +/* -*- c -*-
> + */
> +
> +%#include "internal.h"
> +
> +typedef opaque virLogManagerProtocolUUID[VIR_UUID_BUFLEN];
> +
> +/* Length of long, but not unbounded, strings.
> + * This is an arbitrary limit designed to stop the decoder from trying
> + * to allocate unbounded amounts of memory when fed with a bad message.
> + */
> +const VIR_LOG_MANAGER_PROTOCOL_STRING_MAX = 65536;

This is going to be modified in the next patch. Shouldn't you use the
right value directly here?

[...]


> diff --git a/src/logging/test_virtlogd.aug.in b/src/logging/test_virtlogd.aug.in
> new file mode 100644
> index 0000000..cc659d2
> --- /dev/null
> +++ b/src/logging/test_virtlogd.aug.in
> @@ -0,0 +1,12 @@
> +module Test_virtlogd =
> +  let conf = "log_level = 3
> +log_filters=\"3:remote 4:event\"
> +log_outputs=\"3:syslog:libvirtd\"

libvirtd?

> +log_buffer_size = 64
> +"
> +
> +   test Virtlogd.lns get conf =
> +        { "log_level" = "3" }
> +        { "log_filters" = "3:remote 4:event" }
> +        { "log_outputs" = "3:syslog:libvirtd" }
> +        { "log_buffer_size" = "64" }

^^ see below.

> diff --git a/src/logging/virtlogd.conf b/src/logging/virtlogd.conf
> new file mode 100644
> index 0000000..609f67a
> --- /dev/null
> +++ b/src/logging/virtlogd.conf
> @@ -0,0 +1,67 @@
> +# Master virtlogd daemon configuration file
> +#
> +
> +#################################################################
> +#
> +# Logging controls
> +#
> +
> +# Logging level: 4 errors, 3 warnings, 2 information, 1 debug
> +# basically 1 will log everything possible
> +#log_level = 3
> +
> +# Logging filters:
> +# A filter allows to select a different logging level for a given category
> +# of logs
> +# The format for a filter is one of:
> +#    x:name
> +#    x:+name
> +#      where name is a string which is matched against source file name,
> +#      e.g., "remote", "qemu", or "util/json", the optional "+" prefix
> +#      tells libvirt to log stack trace for each message matching name,
> +#      and x is the minimal level where matching messages should be logged:
> +#    1: DEBUG
> +#    2: INFO
> +#    3: WARNING
> +#    4: ERROR
> +#
> +# Multiple filter can be defined in a single @filters, they just need to be
> +# separated by spaces.
> +#
> +# e.g. to only get warning or errors from the remote layer and only errors
> +# from the event layer:
> +#log_filters="3:remote 4:event"
> +
> +# Logging outputs:
> +# An output is one of the places to save logging information
> +# The format for an output can be:
> +#    x:stderr
> +#      output goes to stderr
> +#    x:syslog:name
> +#      use syslog for the output and use the given name as the ident
> +#    x:file:file_path
> +#      output to a file, with the given filepath

journald is used as default when initializing, so it should be
docummented here.

> +# In all case the x prefix is the minimal level, acting as a filter
> +#    1: DEBUG
> +#    2: INFO
> +#    3: WARNING
> +#    4: ERROR
> +#
> +# Multiple output can be defined, they just need to be separated by spaces.
> +# e.g. to log all warnings and errors to syslog under the virtlogd ident:
> +#log_outputs="3:syslog:virtlogd"
> +#
> +
> +# Log debug buffer size:
> +#
> +# This configuration option is no longer used, since the global
> +# log buffer functionality has been removed. Please configure
> +# suitable log_outputs/log_filters settings to obtain logs.
> +#log_buffer_size = 64a

If it's not used, it should not be part of the new daemon any more.

> +
> +# The maximum number of concurrent client connections to allow
> +# over all sockets combined.
> +# Each running virtual machine will require one open connection
> +# to virtlogd. So 'max_clients' will affect how many VMs can
> +# be run on a host
> +#max_clients = 1024

Should we mention this also in the libvirtd config file?

> diff --git a/src/logging/virtlogd.pod.in b/src/logging/virtlogd.pod.in
> new file mode 100644
> index 0000000..bba7714
> --- /dev/null
> +++ b/src/logging/virtlogd.pod.in
> @@ -0,0 +1,162 @@

[...]

> +
> +=head1 COPYRIGHT
> +
> +Copyright (C) 2006-2015 Red Hat, Inc., and the authors listed in the
> +libvirt AUTHORS file.

2006?

ACK, with suggesed cleanups.
I wasn't entirely thorough when reviewing this since it is 75k of new
stuff.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151118/b8ebed49/attachment-0001.sig>