On Wed, Nov 09, 2016 at 11:22:34AM -0500, John Ferlan wrote:

On 11/01/2016 06:27 AM, Erik Skultety wrote:

...

The reason why we need something like this lies in the daemon's config where we treat the @log_outputs variable (but not just this one) the very same way in cases where the variable was explicitly set to an empty string or wasn't set at all, using some default output in both. The selection of a default output depends on whether the daemon runs daemonized or not. Before the runtime logging APIs can be enabled, we need to make sure that the behaviour will be the same in case someone tries to replace the set of logging outputs with an empty string, hoping that it would turn the logging off. In order to be able to reset the logging output to some default we either need to store the daemon mode or we store a default logging output which we'll be able to fallback to later. This patch goes for the latter by introducing new methods to set and retrieve the default logging output.

The commit message feels like a continuation of the cover and justification for a static virLogDefaultOutput.

The shortened version is - introduce new helpers to handle managing output log file defaults and save/fetch of a default log location. This patch will store the default

All these changes should be usable by lib{virtd|logd|lockd}... Although I didn't quite dig into the details of those...

Sigh, I had it on my mind and then the very next day - puff - I completely forgot about it compiled the patches one last time and sent patches right away :/

...

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virlog.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 2 ++ 3 files changed, 98 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 162fda5..5b0e07d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1877,6 +1877,7 @@ virLogFilterFree; virLogFilterListFree; virLogFilterNew; virLogFindOutput; +virLogGetDefaultOutput; virLogGetDefaultPriority; virLogGetFilters; virLogGetNbFilters; @@ -1895,6 +1896,7 @@ virLogParseOutputs; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; +virLogSetDefaultOutput; virLogSetDefaultPriority; virLogSetFilters; virLogSetFromEnv; diff --git a/src/util/virlog.c b/src/util/virlog.c index 8f831fc..4ac72dc 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -50,6 +50,7 @@ #include "virtime.h" #include "intprops.h" #include "virstring.h" +#include "configmake.h"

/* Journald output is only supported on Linux new enough to expose * htole64. */ @@ -105,6 +106,7 @@ struct _virLogOutput { char *name; };

+static char *virLogDefaultOutput;

After reading through to the end, I could see use for a virLogDefaultFilter too... But that's a different problem. Focus, focus, focus on the current one ;-)!

Well, I'm only doing this because we have to stay consistent with the config file. The defaults for filters on the other hand do not support any defaults, you don't set any, you don't have any. I can imagine we could have the same for the filters as you suggest if libvirt supported something like 'save as default' which then would make sense for both. But since the outputs defaults are hard-coded, the difference in the interpretation of the default in both cases could end up IMHO slightly confusing, but maybe I'm looking at it from the wrong angle.

...

static virLogOutputPtr *virLogOutputs; static size_t virLogNbOutputs;

@@ -146,6 +148,98 @@ virLogUnlock(void) virMutexUnlock(&virLogMutex); }

Two spaces between functions (more than one occurrence)

...

+static int +virLogSetDefaultOutputToStderr(void) +{ + char *tmp = NULL; + if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) + return -1; + + virLogDefaultOutput = tmp; + return 0;

Or more simply

return virAsprintf(&virLogDefaultOutput, ...);

of course on error virLogDefaultOutput = NULL;, which shouldn't matter since this is only ever being done once

Well I can rewrite it, no problem whatsoever, I just looked at it as if I wrote any other function, disregarding the fact that it's going to be called once and once only completely and instead treated it like it's going to be called regularly and so we should not touch caller's reference if we're not 100% sure nothing can wrong anymore.

Also since virLogDefaultPriority is owned here, do you really need the virLogGetDefaultPriority() helper?

Yeah, I certainly don't, true that :).

...

+} + +static int +virLogSetDefaultOutputToJournald(void) +{ + char *tmp = NULL; + virLogPriority priority = virLogDefaultPriority;

here you went directly to the local virLogDefaultPriority...

...

+ + /* By default we don't want to log too much stuff into journald as + * it may employ rate limiting and thus block libvirt execution. */ + if (priority == VIR_LOG_DEBUG) + priority = VIR_LOG_INFO; + + if (virAsprintf(&tmp, "%d:journald", priority) < 0) + return -1; + + virLogDefaultOutput = tmp; + return 0;

similar here return virAsprintf(&virLogDefaultOutput, ...);

...

+} + +static int +virLogSetDefaultOutputToFile(bool privileged)

If this took a param "char const *fname, then the "libvirtd.log" could be replaced with %s using fname. Thus perhaps being reusable for vir{Lock|Log}DaemonSetupLogging.

...

+{ + int ret = -1; + char *tmp = NULL; + char *logdir = NULL; + + if (privileged) { + if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/libvirtd.log", + virLogGetDefaultPriority(),

Again use virLogDefaultPriority

and "libvirt.log" gets replaced by %s with the 3rd param being fname

...

+ LOCALSTATEDIR) < 0) + goto cleanup; + } else { + if (!(logdir = virGetUserCacheDirectory())) + goto cleanup; + + mode_t old_umask = umask(077);

Promote 'mode_t old_umask;' to the top too

...

+ if (virFileMakePath(logdir) < 0) { + umask(old_umask); + goto cleanup; + } + umask(old_umask); + + if (virAsprintf(&tmp, "%d:file:%s/libvirtd.log", + virLogGetDefaultPriority(), logdir) < 0)

Use virLogDefaultPriority - likewise with 3rd param

...

+ goto cleanup; + } + + virLogDefaultOutput = tmp; + tmp = NULL;

similar in here w/r/t virAsprintf(&virLogDefaultOutput, ...)

...

+ ret = 0; + cleanup: + VIR_FREE(tmp); + VIR_FREE(logdir); + return ret; +} + +/* this should be run exactly once at daemon startup, so no locking is + * necessary + */

Add some comments regarding input/output, actions, etc. In fact, I think if you grab the comments (more or less) from existing (and duplicated) code/comments from lib{virtd|logd|lockd} and move it here, massage it to fit the reality you're creating, then that would be fine.

...

+int +virLogSetDefaultOutput(virLogDestination dest, bool privileged)

This could just be how daemonSetupLoggingDefaults gets changed by your patch 3 (although I've kept isatty - reason in patch 3)

Thus you'd have:

virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged)

{ if (!godaemon) return virLogSetDefaultOutputToStderr(privileged);

if (!isatty(STDIN_FILENO) && access("/run/systemd/journal/socket", W_OK) >= 0)) return virLogSetDefaultOutputToJournald(privileged);

return virLogSetDefaultOutputToFile(fname, privileged); }

I was looking at this^^ for quite some time thinking what I'd missed, why hadn't I written it this elegantly and then I realized that unfortunately it is not equivalent to how the daemon behaves now. With your change, the fact STDIN is a tty is only considered when we know we're running as daemon, which isn't the case with the current behaviour where we consider it in the opposite case when we're not running as a daemon but we don't have a tty (some other process spawned us and passed the fds). So eventually, with your proposal you'd go for stderr every time we're not running as daemon which is essentially what I'm proposing, we should really only consider @godaemon, because if someone decides to pass some exotic FDs, it shouldn't really be our problem, the calling process should handle extracting our logging from the FD it passed to us. I also spent a fair amount of time figuring out why commit @eba36a38 didn't consider just @godaemon and I have to say I wasn't successful at all. Erik

...

+{ + switch (dest) { + case VIR_LOG_TO_STDERR: + return virLogSetDefaultOutputToStderr(); + case VIR_LOG_TO_JOURNALD: + return virLogSetDefaultOutputToJournald(); + case VIR_LOG_TO_FILE: + return virLogSetDefaultOutputToFile(privileged); + case VIR_LOG_TO_SYSLOG: + case VIR_LOG_TO_OUTPUT_LAST: + break; + } + + return 0; +} + +char * +virLogGetDefaultOutput(void) +{ + return virLogDefaultOutput; +}

static const char * virLogPriorityString(virLogPriority lvl) diff --git a/src/util/virlog.h b/src/util/virlog.h index 3f2d422..d6eb693 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -189,6 +189,8 @@ void virLogFilterFree(virLogFilterPtr filter); void virLogFilterListFree(virLogFilterPtr *list, int count); int virLogSetOutputs(const char *outputs) ATTRIBUTE_NONNULL(1); int virLogSetFilters(const char *filters); +char *virLogGetDefaultOutput(void); +int virLogSetDefaultOutput(virLogDestination dest, bool privileged);

/* * Internal logging API

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Now that virLog{Get,Set}DefaultOutput routines are introduced we can wire them up to the daemon's logging initialization code. As part of this process, refactor the daemonSetupLoggingDefaults method, since the code isn't particularly easy to read (due to the condition below). However, this refactor changes the logic of the selection of the default logging output in the way demonstrated below. Long story short, we should really only care if we're running daemonized or not, disregarding the fact of (not) having a TTY completely as that should be of the libvirtd's parent concern (what FD it will pass to it).

See commit id 'eba36a3878' regarding !isatty(STDIN_FILENO) - it's not 100% clear to me whether we could just remove it. I think we should keep it.

Before: if (godaemon || !hasTTY): if (journald): use journald

if (godaemon): if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr

After: if (godaemon): if (journald): use journald

else: if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/libvirtd.c | 86 ++++++++++++++----------------------------------------- 1 file changed, 21 insertions(+), 65 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9a5f193..3af4ea1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c

Since you added configmake.h to virlog.c in patch 2 and remove what was necessary for here, it's no longer needed here...

@@ -660,65 +660,24 @@ daemonSetupNetworking(virNetServerPtr srv, static int daemonSetupLoggingDefaults(bool godaemon, bool privileged) {

This ends up not being needed

- if (virLogGetOutputs() == 0 && - (godaemon || !isatty(STDIN_FILENO))) {

- char *tmp; + /* If we're running as a daemon, the try to direct the output to systemd + * journal first (if it exists), otherwise fallback to libvirtd.log. If not + * running as a daemon, use stderr as default. + */ + if (!godaemon) { + if (virLogSetDefaultOutput(VIR_LOG_TO_STDERR, privileged) < 0) + return -1; + } else { if (access("/run/systemd/journal/socket", W_OK) >= 0) { - virLogPriority priority = virLogGetDefaultPriority(); - - /* By default we don't want to log too much stuff into journald as - * it may employ rate limiting and thus block libvirt execution. */ - if (priority == VIR_LOG_DEBUG) - priority = VIR_LOG_INFO; - - if (virAsprintf(&tmp, "%d:journald", priority) < 0) - goto error; - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - } - - if (virLogGetOutputs() == 0) { - char *tmp = NULL; - - if (godaemon) { - if (privileged) { - if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/libvirtd.log", - virLogGetDefaultPriority(), - LOCALSTATEDIR) == -1) - goto error; - } else { - char *logdir = virGetUserCacheDirectory(); - mode_t old_umask; - - if (!logdir) - goto error; - - old_umask = umask(077); - if (virFileMakePath(logdir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(&tmp, "%d:file:%s/libvirtd.log", - virLogGetDefaultPriority(), logdir) == -1) { - VIR_FREE(logdir); - goto error; - } - VIR_FREE(logdir); - } + if (virLogSetDefaultOutput(VIR_LOG_TO_JOURNALD, privileged) < 0) + return -1; } else { - if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) - goto error; + if (virLogSetDefaultOutput(VIR_LOG_TO_FILE, privileged) < 0) + return -1; } - virLogSetOutputs(tmp); - VIR_FREE(tmp); }

return 0; - error: - return -1; }

/* @@ -733,6 +692,9 @@ daemonSetupLogging(struct daemonConfig *config, bool verbose, bool godaemon) { + if (daemonSetupLoggingDefaults(godaemon, privileged) < 0) + return -1; +

s/(godaemon/("libvirtd.log", godaemon/ Although I don't think this is the exact right place... I think it should be moved to just before the virLogSetFromEnv(). Also I think the command line override for --verbose should be moved up as well before the call to set the log defaults... Thus it'd be: if (config->log_level != 0) virLogSetDefaultPriority(config->log_level); /* * Command line override for --verbose */ if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) virLogSetDefaultPriority(VIR_LOG_INFO); if (daemonSetupLoggingDefaults("libvirtd.log", godaemon, privileged) < 0) return -1; This would be repeated for libvirtd.c, lock_daemon.c, and log_daemon.c

virLogReset();

/* @@ -767,20 +729,14 @@ daemonSetupLogging(struct daemonConfig *config, virLogSetDefaultPriority(VIR_LOG_INFO);

/* - * If no defined outputs, and either running - * as daemon or not on a tty, then first try - * to direct it to the systemd journal - * (if it exists), otherwise fallback to libvirtd.log. If both not running - * as daemon and having a tty, use stderr as default. - */ - if (virLogGetNbOutputs() == 0 && - daemonSetupLoggingDefaults(godaemon, privileged) < 0) - goto error; + * If there are no outputs defined, use the default one */ + if (!virLogGetNbOutputs()) {

I'd rather see "if (virLogGetNbOutputs() == 0)" Save the ! for NULL ..

+ char *tmp = virLogGetDefaultOutput(); + virLogSetOutputs(tmp);

just go direct: virLogSetOutputs(virLogGetDefaultOutput()) John

+ VIR_FREE(tmp); + }

return 0; - - error: - return -1; }

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Enable libvirt users to query logging output settings.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 52 +++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 ++++++++++++- src/admin/admin_remote.c | 43 ++++++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 +++++++ src/libvirt-admin.c | 39 +++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/daemon/admin.c b/daemon/admin.c index a3c8b89..b31b125 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -383,4 +383,56 @@ adminDispatchServerSetClientLimits(virNetServerPtr server ATTRIBUTE_UNUSED, virObjectUnref(srv); return rv; } +

/* Returns: Count of outputs? */

+static int +adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) +{ + char *tmp = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if ((ret = virLogGetNbOutputs()) > 0) { + if (!(tmp = virLogGetOutputs())) + return -1; + } else { + /* there were no outputs defined, return an empty string */ + if (!tmp) {

How would tmp have anything?

+ if (VIR_ALLOC(tmp) < 0) + return -1; + memset(tmp, 0, 1);

if (VIR_STRDUP(tmp, "") < 0) return -1; ret = 1;

+ } + } + + *outputs = tmp; + return ret; +} + +static int +adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + admin_connect_get_logging_outputs_args *args, + admin_connect_get_logging_outputs_ret *ret) +{ + char *outputs = NULL; + int noutputs = 0; + int rv = -1; + + if ((noutputs = adminConnectGetLoggingOutputs(&outputs, args->flags) < 0)) + goto cleanup; + + if (VIR_STRDUP(ret->outputs, outputs) < 0) + goto cleanup;

Would this be better served as VIR_STEAL_PTR(ret->outputs, outputs);

+ + ret->noutputs = noutputs; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr);

rerr not really UNUSED then? The failure path could just be inlined thus removing need for cleanup label and local rv

+ VIR_FREE(outputs); + return rv; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index c810be3..46d2155 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -404,6 +404,10 @@ int virAdmServerSetClientLimits(virAdmServerPtr srv, int nparams, unsigned int flags);

+int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 5963233..237632a 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -183,6 +183,15 @@ struct admin_server_set_client_limits_args { unsigned int flags; };

+struct admin_connect_get_logging_outputs_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_outputs_ret { + admin_nonnull_string outputs; + unsigned int noutputs; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -268,5 +277,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13 + ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 10a3b18..de3662c 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -429,3 +429,46 @@ remoteAdminServerSetClientLimits(virAdmServerPtr srv, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags) +{ + int rv = -1; + char *tmp_outputs = NULL; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_get_logging_outputs_args args; + admin_connect_get_logging_outputs_ret ret; + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + virObjectLock(priv); + + if (call(conn, + 0, + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS, + (xdrproc_t) xdr_admin_connect_get_logging_outputs_args, + (char *) &args, + (xdrproc_t) xdr_admin_connect_get_logging_outputs_ret, + (char *) &ret) == -1) + goto done; + + if (outputs) {

virAdmConnectGetLoggingOutputs checks for NonNullArg(outputs) So this doesn't seem necessary...

+ if (VIR_STRDUP(tmp_outputs, ret.outputs) < 0) + goto cleanup;

Similar - VIR_STEAL_PTR(*outputs, ret.outputs); should work, right?

+ + *outputs = tmp_outputs; + tmp_outputs = NULL; + } + + rv = ret.noutputs; + + cleanup: + xdr_free((xdrproc_t) xdr_admin_connect_get_logging_outputs_ret, (char *) &ret); + + done: + virObjectUnlock(priv); + return rv; +} diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 1437d9e..096bf5a 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -127,6 +127,13 @@ struct admin_server_set_client_limits_args { } params; u_int flags; }; +struct admin_connect_get_logging_outputs_args { + u_int flags; +}; +struct admin_connect_get_logging_outputs_ret { + admin_nonnull_string outputs; + u_int noutputs; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -141,4 +148,5 @@ enum admin_procedure { ADMIN_PROC_CLIENT_CLOSE = 11, ADMIN_PROC_SERVER_GET_CLIENT_LIMITS = 12, ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 88eef54..264411f 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1106,3 +1106,42 @@ virAdmServerSetClientLimits(virAdmServerPtr srv, virDispatchError(NULL); return ret; } + +/** + * virAdmConnectGetLoggingOutputs: + * @conn: pointer to an active admin connection + * @outputs: pointer to a variable to store a string containing all currently + * defined logging outputs on daemon (allocated automatically) + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves a list of currently installed logging outputs. Outputs returned + * are contained within an automatically allocated string and delimited by + * spaces. The format of each output conforms to the format described in + * daemon's configuration file (e.g. libvirtd.conf).

space between paragraphs

+ * To retrieve individual outputs, additional parsing needs to be done by the + * caller. Caller is also responsible for freeing @outputs correctly. + * + * Returns the count of outputs in @outputs, or -1 in case of an error. + */ +int +virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(outputs, error);

So this seems to go against the remoteAdminConnectGetLoggingOutputs "if (outputs)" needs/check.

+ + if ((ret = remoteAdminConnectGetLoggingOutputs(conn, outputs, + flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index 8c173ab..c95aa3c 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -10,6 +10,8 @@ xdr_admin_client_close_args; xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_ret; +xdr_admin_connect_get_logging_outputs_args; +xdr_admin_connect_get_logging_outputs_ret; xdr_admin_connect_list_servers_args; xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 2de28e9..c1420ee 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -38,4 +38,5 @@ LIBVIRT_ADMIN_2.0.0 { virAdmClientClose; virAdmServerGetClientLimits; virAdmServerSetClientLimits; + virAdmConnectGetLoggingOutputs; };

Doesn't this need to be a new stanza similar to libvirt_public.syms, e.g.: LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; } LIBVIRT_ADMIN_2.0.0; with obvious additions for the next 3 patches. John

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Enable libvirt users to query logging filter settings.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 52 +++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 ++++++++++++- src/admin/admin_remote.c | 43 ++++++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 +++++++ src/libvirt-admin.c | 40 +++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 165 insertions(+), 1 deletion(-)

I assume there will be similar comments here as to the previous patch... I won't repeat them...

diff --git a/daemon/admin.c b/daemon/admin.c index b31b125..c0598e0 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -409,6 +409,30 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) }

static int +adminConnectGetLoggingFilters(char **filters, unsigned int flags) +{ + char *tmp = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if ((ret = virLogGetNbFilters()) > 0) { + if (!(tmp = virLogGetFilters())) + return -1;

Indent return -1;

+ } else { + /* there were no filters defined, return an empty string */ + if (!tmp) { + if (VIR_ALLOC(tmp) < 0) + return -1; + memset(tmp, 0, 1); + } + } + + *filters = tmp; + return ret; +} + +static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, @@ -435,4 +459,32 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, VIR_FREE(outputs); return rv; } + +static int +adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + admin_connect_get_logging_filters_args *args, + admin_connect_get_logging_filters_ret *ret) +{ + char *filters = NULL; + int nfilters = 0; + int rv = -1; + + if ((nfilters = adminConnectGetLoggingFilters(&filters, args->flags)) < 0) + goto cleanup; + + if (VIR_STRDUP(ret->filters, filters) < 0) + goto cleanup; + + ret->nfilters = nfilters; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + VIR_FREE(filters); + return rv; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 46d2155..d76ac20 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -408,6 +408,10 @@ int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, char **outputs, unsigned int flags);

+int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 237632a..69aeb54 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -192,6 +192,15 @@ struct admin_connect_get_logging_outputs_ret { unsigned int noutputs; };

+struct admin_connect_get_logging_filters_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_filters_ret { + admin_nonnull_string filters; + unsigned int nfilters; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -282,5 +291,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14 + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index de3662c..ec128f6 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -472,3 +472,46 @@ remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int rv = -1; + char *tmp_filters = NULL; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_get_logging_filters_args args; + admin_connect_get_logging_filters_ret ret; + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + virObjectLock(priv); + + if (call(conn, + 0, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS, + (xdrproc_t) xdr_admin_connect_get_logging_filters_args, + (char *) &args, + (xdrproc_t) xdr_admin_connect_get_logging_filters_ret, + (char *) &ret) == -1) + goto done; + + if (filters) { + if (VIR_STRDUP(tmp_filters, ret.filters) < 0) + goto cleanup; + + *filters = tmp_filters; + tmp_filters = NULL; + } + + rv = ret.nfilters; + + cleanup: + xdr_free((xdrproc_t) xdr_admin_connect_get_logging_filters_ret, (char *) &ret); + + done: + virObjectUnlock(priv); + return rv; +} diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 096bf5a..41d065d 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -134,6 +134,13 @@ struct admin_connect_get_logging_outputs_ret { admin_nonnull_string outputs; u_int noutputs; }; +struct admin_connect_get_logging_filters_args { + u_int flags; +}; +struct admin_connect_get_logging_filters_ret { + admin_nonnull_string filters; + u_int nfilters; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -149,4 +156,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_GET_CLIENT_LIMITS = 12, ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 264411f..61a538c 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1145,3 +1145,43 @@ virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectGetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a variable to store a string containing all currently + * defined logging filters on daemon (allocated automatically) + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves a list of currently installed logging filters. Filters returned + * are contained within an automatically allocated string and delimited by + * spaces. The format of each filter conforms to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * To retrieve individual filters, additional parsing needs to be done by the + * caller. Caller is also responsible for freeing @filters correctly. + * + * Returns the number of filters returned in @filters, or -1 in case of + * an error. + */ +int +virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(filters, error); + + if ((ret = remoteAdminConnectGetLoggingFilters(conn, filters, + flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index c95aa3c..b36880d 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -10,6 +10,8 @@ xdr_admin_client_close_args; xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_ret; +xdr_admin_connect_get_logging_filters_args; +xdr_admin_connect_get_logging_filters_ret; xdr_admin_connect_get_logging_outputs_args; xdr_admin_connect_get_logging_outputs_ret; xdr_admin_connect_list_servers_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index c1420ee..e6ee0e8 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -38,5 +38,6 @@ LIBVIRT_ADMIN_2.0.0 { virAdmClientClose; virAdmServerGetClientLimits; virAdmServerSetClientLimits; + virAdmConnectGetLoggingFilters;

Similar to 4/8 John

virAdmConnectGetLoggingOutputs; };

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Enable libvirt users to modify daemon's logging output settings from outside. If an empty set is passed, a default logging output will be used the same way as it would be in case writing an empty string to the libvirtd.conf

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 21 +++++++++++---------- include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 12 +++++++++++- src/admin_protocol-structs | 5 +++++ src/libvirt-admin.c | 36 ++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + src/util/virlog.c | 5 ++++- 8 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/daemon/admin.c b/daemon/admin.c index c0598e0..79961b2 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -392,17 +392,8 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags)

virCheckFlags(0, -1);

- if ((ret = virLogGetNbOutputs()) > 0) { - if (!(tmp = virLogGetOutputs())) + if ((ret = virLogGetNbOutputs()) > 0 && !(tmp = virLogGetOutputs())) return -1;

Indention is off

- } else { - /* there were no outputs defined, return an empty string */ - if (!tmp) { - if (VIR_ALLOC(tmp) < 0) - return -1; - memset(tmp, 0, 1); - } - }

*outputs = tmp; return ret; @@ -433,6 +424,16 @@ adminConnectGetLoggingFilters(char **filters, unsigned int flags) }

static int +adminConnectSetLoggingOutputs(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *outputs, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetOutputs(outputs); +} + +static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index d76ac20..aa33fef 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -412,6 +412,10 @@ int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, char **filters, unsigned int flags);

+int virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, + const char *outputs, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 69aeb54..4a05928 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -201,6 +201,11 @@ struct admin_connect_get_logging_filters_ret { unsigned int nfilters; };

+struct admin_connect_set_logging_outputs_args { + admin_nonnull_string outputs; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -296,5 +301,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15 + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 41d065d..cbc99e3 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -141,6 +141,10 @@ struct admin_connect_get_logging_filters_ret { admin_nonnull_string filters; u_int nfilters; }; +struct admin_connect_set_logging_outputs_args { + admin_nonnull_string outputs; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -157,4 +161,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 61a538c..01ae26c 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1185,3 +1185,39 @@ virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectSetLoggingOutputs: + * @conn: pointer to an active admin connection + * @outputs: pointer to a string containing a list of outputs to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Redefine the existing (set of) outputs(s) with a new one specified in + * @outputs. If multiple outputs are specified, they need to be delimited by + * spaces. The format of each output must conform to the format described in + * daemon's configuration file (e.g. libvirtd.conf).

If an empty sring ("") is passed, then a default will be used (restated more cleanly of course!)

+ * + * Returns 0 if the new output or the set of outputs has been defined + * successfully, or -1 in case of an error. + */ +int +virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, + const char *outputs, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, outputs=%s, flags=%x", conn, outputs, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(outputs, error);

This won't allow NULL outputs...

+ + if ((ret = remoteAdminConnectSetLoggingOutputs(conn, outputs, flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index b36880d..d1ca034 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -19,6 +19,7 @@ xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; xdr_admin_connect_lookup_server_ret; xdr_admin_connect_open_args; +xdr_admin_connect_set_logging_outputs_args; xdr_admin_server_get_client_limits_args; xdr_admin_server_get_client_limits_ret; xdr_admin_server_get_threadpool_parameters_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index e6ee0e8..d39de42 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -40,4 +40,5 @@ LIBVIRT_ADMIN_2.0.0 { virAdmServerSetClientLimits; virAdmConnectGetLoggingFilters; virAdmConnectGetLoggingOutputs; + virAdmConnectSetLoggingOutputs;

see patch 4 w/r/t new stanza...

}; diff --git a/src/util/virlog.c b/src/util/virlog.c index 4ac72dc..d04a461 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1820,6 +1820,8 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) * @outputs: string defining a (set of) output(s) * * Replaces the current set of defined outputs with a new set of outputs. + * Should the set be empty, a default output is used according to the daemon's + * runtime attributes. * * Returns 0 on success or -1 in case of an error. */ @@ -1828,12 +1830,13 @@ virLogSetOutputs(const char *src) { int ret = -1; int noutputs = 0; + const char *outputstr = *src ? src : virLogGetDefaultOutput();

Not sure this is right - virAdmConnectSetLoggingOutputs says 'outputs' must be NonNull. I assume the generated remoteAdminConnectSetLoggingOutputs will end up calling adminConnectSetLoggingOutputs with a NonNull 'outputs'. Thus this code gets called with NonNull outputs. Anyway, what you're really looking to do is check if the contents of 'outputs' is empty, then use some default value. In this case, since this code "owns" virLogDefaultOutput is there really a reason to call the API? NB: even though virlog.h says outputs cannot be passed as NULL that's somewhat of a misnomer since all the compiler is checking is that the argument in the call stack isn't NULL - it doesn't check if the argument being passed in actually NULL. John

virLogOutputPtr *outputs = NULL;

if (virLogInitialize() < 0) return -1;

- if ((noutputs = virLogParseOutputs(src, &outputs)) < 0) + if ((noutputs = virLogParseOutputs(outputstr, &outputs)) < 0) goto cleanup;

if (virLogDefineOutputs(outputs, noutputs) < 0)

On 11/22/2016 11:29 AM, Erik Skultety wrote:

...

...

}; diff --git a/src/util/virlog.c b/src/util/virlog.c index 4ac72dc..d04a461 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1820,6 +1820,8 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) * @outputs: string defining a (set of) output(s) * * Replaces the current set of defined outputs with a new set of outputs. + * Should the set be empty, a default output is used according to the daemon's + * runtime attributes. * * Returns 0 on success or -1 in case of an error. */ @@ -1828,12 +1830,13 @@ virLogSetOutputs(const char *src) { int ret = -1; int noutputs = 0; + const char *outputstr = *src ? src : virLogGetDefaultOutput();

Not sure this is right - virAdmConnectSetLoggingOutputs says 'outputs' must be NonNull.

I assume the generated remoteAdminConnectSetLoggingOutputs will end up calling adminConnectSetLoggingOutputs with a NonNull 'outputs'.

Thus this code gets called with NonNull outputs.

So, what exactly are you proposing? Because what I understand from it now is that I should probably allow passing NULL outputs in virAdmConnectSetLoggingOutputs as a way of resetting the outputs to our defaults (as an addition to "").

I wonder if I was reading this as "src ? src : virLogGetDefaultOutput()" Been too long for me to recall for sure, but I do see the *src now and sure that does what you want and I think what I wrote (so what was a I thinking?)...

...

Anyway, what you're really looking to do is check if the contents of 'outputs' is empty, then use some default value. In this case, since this code "owns" virLogDefaultOutput is there really a reason to call the API?

True, I certainly don't have to make the call.

Usage of the API isn't necessary, but it can be done - some prefer one way over another.

...

NB: even though virlog.h says outputs cannot be passed as NULL that's somewhat of a misnomer since all the compiler is checking is that the argument in the call stack isn't NULL - it doesn't check if the argument being passed in actually NULL.

Yes, but this should be actual check for NULL should be the case when we're handling user-passed values in which case as it is right now, there's no need, user can't pass NULL and in all the other cases, our internal callers should respect the ATTRIBUTE_NONNULL constraint. Of course, if you were suggesting to actually allow users to pass NULL to the public API^^, then yes, this has to be adjusted properly.

An empty string is fine and no need to check for NULL too, unless you think setting up an empty outputs would be useful for some other designation (but we can leave that to the future). John

Thanks, Erik

...

John

...

virLogOutputPtr *outputs = NULL;

if (virLogInitialize() < 0) return -1;

- if ((noutputs = virLogParseOutputs(src, &outputs)) < 0) + if ((noutputs = virLogParseOutputs(outputstr, &outputs)) < 0) goto cleanup;

if (virLogDefineOutputs(outputs, noutputs) < 0)

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Enable libvirt users to modify logging filters of a daemon from outside.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 10 ++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 12 +++++++++++- src/admin_protocol-structs | 5 +++++ src/libvirt-admin.c | 36 ++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/daemon/admin.c b/daemon/admin.c index 79961b2..b66ccd8 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -434,6 +434,16 @@ adminConnectSetLoggingOutputs(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, }

static int +adminConnectSetLoggingFilters(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *filters, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetFilters(filters); +} + +static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index aa33fef..161727e 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -416,6 +416,10 @@ int virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, const char *outputs, unsigned int flags);

+int virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 4a05928..4eef088 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -206,6 +206,11 @@ struct admin_connect_set_logging_outputs_args { unsigned int flags; };

+struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -306,5 +311,10 @@ enum admin_procedure { /** * @generate: both */ - ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16 + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index cbc99e3..1974c07 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -145,6 +145,10 @@ struct admin_connect_set_logging_outputs_args { admin_nonnull_string outputs; u_int flags; }; +struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -162,4 +166,5 @@ enum admin_procedure { ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 01ae26c..c74e6b9 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1221,3 +1221,39 @@ virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectSetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a string containing a list of filters to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Redefine the existing (set of) filter(s) with a new one specified in + * @filters. If multiple filters are specified, they need to be delimited by + * spaces. The format of each filter must conform to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + *

So you didn't want to write the code that would allow resetting filters back to their defaults? Or is that next? IOW: I can see value for altering the filters in real time and then wanting to reset them back to whatever was originally set at daemon startup.

+ * Returns 0 if the new filter or the set of filters has been defined + * successfully, or -1 in case of an error. + */ +int +virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(filters, error); + + if ((ret = remoteAdminConnectSetLoggingFilters(conn, filters, flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index d1ca034..22b7167 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -19,6 +19,7 @@ xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; xdr_admin_connect_lookup_server_ret; xdr_admin_connect_open_args; +xdr_admin_connect_set_logging_filters_args; xdr_admin_connect_set_logging_outputs_args; xdr_admin_server_get_client_limits_args; xdr_admin_server_get_client_limits_ret; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d39de42..b4aee98 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -40,5 +40,6 @@ LIBVIRT_ADMIN_2.0.0 { virAdmServerSetClientLimits; virAdmConnectGetLoggingFilters; virAdmConnectGetLoggingOutputs; + virAdmConnectSetLoggingFilters;

Similar stanza comment from patch 6 John

virAdmConnectSetLoggingOutputs; };

On 11/22/2016 11:29 AM, Erik Skultety wrote:

On Wed, Nov 09, 2016 at 11:26:26AM -0500, John Ferlan wrote:

...

On 11/01/2016 06:27 AM, Erik Skultety wrote:

...

Enable libvirt users to modify logging filters of a daemon from outside.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 10 ++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 12 +++++++++++- src/admin_protocol-structs | 5 +++++ src/libvirt-admin.c | 36 ++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/daemon/admin.c b/daemon/admin.c index 79961b2..b66ccd8 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -434,6 +434,16 @@ adminConnectSetLoggingOutputs(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, }

static int +adminConnectSetLoggingFilters(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *filters, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetFilters(filters); +} + +static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index aa33fef..161727e 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -416,6 +416,10 @@ int virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, const char *outputs, unsigned int flags);

+int virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 4a05928..4eef088 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -206,6 +206,11 @@ struct admin_connect_set_logging_outputs_args { unsigned int flags; };

+struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -306,5 +311,10 @@ enum admin_procedure { /** * @generate: both */ - ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16 + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index cbc99e3..1974c07 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -145,6 +145,10 @@ struct admin_connect_set_logging_outputs_args { admin_nonnull_string outputs; u_int flags; }; +struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -162,4 +166,5 @@ enum admin_procedure { ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 01ae26c..c74e6b9 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1221,3 +1221,39 @@ virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectSetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a string containing a list of filters to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Redefine the existing (set of) filter(s) with a new one specified in + * @filters. If multiple filters are specified, they need to be delimited by + * spaces. The format of each filter must conform to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + *

So you didn't want to write the code that would allow resetting filters back to their defaults? Or is that next?

See my reply to 2/8, we should either fallback to some hard-coded value like with the outputs or not at all, since that would introduce certain level of confusion like what do we actually default to - a libvirt hard-coded value or the initial user-provided value.

So passing NULL could have a different result than passing ""?... Hmmm. In any case, this is probably what prompted me to note something in patch 2 about virLogDefaultFilter. John

Erik

...

IOW: I can see value for altering the filters in real time and then wanting to reset them back to whatever was originally set at daemon startup.

...

+ * Returns 0 if the new filter or the set of filters has been defined + * successfully, or -1 in case of an error. + */ +int +virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(filters, error); + + if ((ret = remoteAdminConnectSetLoggingFilters(conn, filters, flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index d1ca034..22b7167 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -19,6 +19,7 @@ xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; xdr_admin_connect_lookup_server_ret; xdr_admin_connect_open_args; +xdr_admin_connect_set_logging_filters_args; xdr_admin_connect_set_logging_outputs_args; xdr_admin_server_get_client_limits_args; xdr_admin_server_get_client_limits_ret; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d39de42..b4aee98 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -40,5 +40,6 @@ LIBVIRT_ADMIN_2.0.0 { virAdmServerSetClientLimits; virAdmConnectGetLoggingFilters; virAdmConnectGetLoggingOutputs; + virAdmConnectSetLoggingFilters;

Similar stanza comment from patch 6

John

...

virAdmConnectSetLoggingOutputs; };

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

On 11/01/2016 06:27 AM, Erik Skultety wrote:

Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce dmn-log-info and dmn-log-define commands.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)

virt-admin.pod? It seems you're defining a new set of configuration alteration comments, so the virt-admin.pod would have a new "CONFIGURATION" section and all the commands could be prefixed with "cfg-" - thoughts? Then you'd have: cfg-log-outputs [outputs] where if [outputs] was provided, then you have a SET function while if not you have a GET function Naturally then you'd have: cfg-log-filters [filters] and you could also have: cfg-log-level [level] which would allow adjustment of that too. OK - so I'm looking really far forward.

diff --git a/tools/virt-admin.c b/tools/virt-admin.c index b1e0c49..0bada05 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,135 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

+/* ------------------- + * Command dmn-log-info

Is this dmn- or daemon- I know this was discussed at KVM Forum, but I've forgotten...

+ * ------------------- + */ +static const vshCmdInfo info_dmn_log_info[] = { + {.name = "help", + .data = N_("view daemon's current logging settings") + }, + {.name = "desc", + .data = N_("Returns all currently active logging settings on daemon. " + "These include global logging level, logging filters and " + "logging outputs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_dmn_log_info[] = { + {.name = "outputs", + .type = VSH_OT_BOOL, + .help = N_("query logging outputs") + }, + {.name = "filters", + .type = VSH_OT_BOOL, + .help = N_("query logging filters") + }, + {.name = NULL} +}; + +static bool +cmdDmnLogInfo(vshControl *ctl, const vshCmd *cmd) +{ + bool optOutputs = vshCommandOptBool(cmd, "outputs"); + bool optFilters = vshCommandOptBool(cmd, "filters"); + bool all = optOutputs + optFilters == 0; + int nfilters, noutputs; + char *filters, *outputs; + vshAdmControlPtr priv = ctl->privData; + + if (all || optFilters) { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + } + + if (all || optOutputs) { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging outputs information")); + return false; + } + } + + if (all || optFilters) { + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters); + } + + if (all || optOutputs) { + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs); + } + + return true; +} + +/* ------------------------- + * Command daemon-log-define

Is the dmn- or daemon-

+ * ------------------------- + */ +static const vshCmdInfo info_dmn_log_define[] = { + {.name = "help", + .data = N_("change daemon's logging settings") + }, + {.name = "desc", + .data = N_("Defines and installs a new set of logging settings on a daemon. " + "These include global logging level, logging filters and " + "logging outputs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_dmn_log_define[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("comma separated list of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("comma separated list of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDmnLogDefine(vshControl *ctl, const vshCmd *cmd) +{ + const char *filters = NULL; + const char *outputs = NULL; + bool optOutputs = vshCommandOptBool(cmd, "outputs"); + bool optFilters = vshCommandOptBool(cmd, "filters"); + vshAdmControlPtr priv = ctl->privData; + + if (!(optOutputs + optFilters)) { + vshError(ctl, _("At least one of options --outputs, --filters " + "is mandatory")); + return false; + } + + if (optFilters && + (vshCommandOptStringReq(ctl, cmd, "filters", &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + + if (optOutputs && + (vshCommandOptStringReq(ctl, cmd, "outputs", &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1311,6 +1440,12 @@ static const vshCmdDef monitoringCmds[] = { .info = info_srv_clients_info, .flags = 0 }, + {.name = "daemon-log-info", + .handler = cmdDmnLogInfo, + .opts = opts_dmn_log_info, + .info = info_dmn_log_info, + .flags = 0 + }, {.name = NULL} };

@@ -1341,6 +1476,12 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-define", + .handler = cmdDmnLogDefine, + .opts = opts_dmn_log_define, + .info = info_dmn_log_define, + .flags = 0 + }, {.name = NULL} };

On 11/22/2016 11:29 AM, Erik Skultety wrote:

On Wed, Nov 09, 2016 at 11:26:31AM -0500, John Ferlan wrote:

...

On 11/01/2016 06:27 AM, Erik Skultety wrote:

...

Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce dmn-log-info and dmn-log-define commands.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)

virt-admin.pod?

Shame on me. I'll fix this in the next version right after we reach a consensus on all the patches.

...

It seems you're defining a new set of configuration alteration comments, so the virt-admin.pod would have a new "CONFIGURATION" section and all the commands could be prefixed with "cfg-" - thoughts?

We already have some cfg- stuff implemented under server- and client-, not mentioning that we already have some aliases too, so I think it's a bit late for this now that we follow the philosophy of placing server-related cfg stuff under server- and client-related stuff under client-, so I think having daemon-generic cfg stuff under daemon- might be reasonable.

Fair enough - no sense in making a new class of command abbreviations. John

Erik

...

Then you'd have:

cfg-log-outputs [outputs]

where if [outputs] was provided, then you have a SET function while if not you have a GET function

Naturally then you'd have:

cfg-log-filters [filters]

and you could also have:

cfg-log-level [level]

which would allow adjustment of that too.

OK - so I'm looking really far forward.

...

diff --git a/tools/virt-admin.c b/tools/virt-admin.c index b1e0c49..0bada05 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,135 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

+/* ------------------- + * Command dmn-log-info

Is this dmn- or daemon-

I know this was discussed at KVM Forum, but I've forgotten...

...

+ * ------------------- + */ +static const vshCmdInfo info_dmn_log_info[] = { + {.name = "help", + .data = N_("view daemon's current logging settings") + }, + {.name = "desc", + .data = N_("Returns all currently active logging settings on daemon. " + "These include global logging level, logging filters and " + "logging outputs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_dmn_log_info[] = { + {.name = "outputs", + .type = VSH_OT_BOOL, + .help = N_("query logging outputs") + }, + {.name = "filters", + .type = VSH_OT_BOOL, + .help = N_("query logging filters") + }, + {.name = NULL} +}; + +static bool +cmdDmnLogInfo(vshControl *ctl, const vshCmd *cmd) +{ + bool optOutputs = vshCommandOptBool(cmd, "outputs"); + bool optFilters = vshCommandOptBool(cmd, "filters"); + bool all = optOutputs + optFilters == 0; + int nfilters, noutputs; + char *filters, *outputs; + vshAdmControlPtr priv = ctl->privData; + + if (all || optFilters) { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + } + + if (all || optOutputs) { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging outputs information")); + return false; + } + } + + if (all || optFilters) { + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters); + } + + if (all || optOutputs) { + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs); + } + + return true; +} + +/* ------------------------- + * Command daemon-log-define

Is the dmn- or daemon-

...

+ * ------------------------- + */ +static const vshCmdInfo info_dmn_log_define[] = { + {.name = "help", + .data = N_("change daemon's logging settings") + }, + {.name = "desc", + .data = N_("Defines and installs a new set of logging settings on a daemon. " + "These include global logging level, logging filters and " + "logging outputs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_dmn_log_define[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("comma separated list of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("comma separated list of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDmnLogDefine(vshControl *ctl, const vshCmd *cmd) +{ + const char *filters = NULL; + const char *outputs = NULL; + bool optOutputs = vshCommandOptBool(cmd, "outputs"); + bool optFilters = vshCommandOptBool(cmd, "filters"); + vshAdmControlPtr priv = ctl->privData; + + if (!(optOutputs + optFilters)) { + vshError(ctl, _("At least one of options --outputs, --filters " + "is mandatory")); + return false; + } + + if (optFilters && + (vshCommandOptStringReq(ctl, cmd, "filters", &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + + if (optOutputs && + (vshCommandOptStringReq(ctl, cmd, "outputs", &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1311,6 +1440,12 @@ static const vshCmdDef monitoringCmds[] = { .info = info_srv_clients_info, .flags = 0 }, + {.name = "daemon-log-info", + .handler = cmdDmnLogInfo, + .opts = opts_dmn_log_info, + .info = info_dmn_log_info, + .flags = 0 + }, {.name = NULL} };

@@ -1341,6 +1476,12 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-define", + .handler = cmdDmnLogDefine, + .opts = opts_dmn_log_define, + .info = info_dmn_log_define, + .flags = 0 + }, {.name = NULL} };

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