On 11/01/2016 06:27 AM, Erik Skultety wrote: Changes in patch 2 resolve a couple of things I noticed below regarding leaked 'logdir' (if MakeFilePath fails) and usage of == -1 rather than < 0 for virAsprintf calls. While I understand it's "outside" the scope of what you're trying to do by adding new admin commands; however, lock_daemon and log_daemon have a lot of "similarities" that I think should use the same code - IOW: I think it's worthwhile to adjust them all. After doing some thinking perhaps patch2 should become patch1 with one adjustment - see my comments there. Then patch2 would adjust libvirtd, lock_daemon, and log_daemon to call virLogSetDefaultOutput as done in patch3 (passing fname of libvirtd.log, virtlogd.log, or virtlockd.log) See my comments in patch 2 w/r/t virLogSetDefaultOutput FWIW: virLogGetOutputs returns char*... This used to be a virLogGetNbOutputs call too... In any case, "currently" the only way into this helper is "if virLogGetNbOutputs() == 0", so in a way one could say this check is unnecessary... Interesting... If this is successful, that means the next hunk doesn't need to be run... thus we could have just returned 0 here. If my assumption above is true, then no need to get the number of outputs... although now moot with my other suggestions... [1] Although moot w/ my latest suggestion, there's a hunk of comments just before this that should have been moved prior to the new function as they make no sense here any more and they get deleted in patch 3. These comments (in some form) would be usable w/ my new suggestion for how virLogSetDefaultOutput could be implemented. The one thing to watch for is changing the 'libvirtd.log' in the comment to @fname (you'll note virtlogd.c and virtlockd.c has essentially copied the comments without changing the file name in the comment ironically). Although moot now, but s/goto error/return -1/ and remove error: here obviously. John

On 11/01/2016 06:27 AM, Erik Skultety wrote: 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... 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 ;-)! Two spaces between functions (more than one occurrence) Or more simply return virAsprintf(&virLogDefaultOutput, ...); of course on error virLogDefaultOutput = NULL;, which shouldn't matter since this is only ever being done once Also since virLogDefaultPriority is owned here, do you really need the virLogGetDefaultPriority() helper? here you went directly to the local virLogDefaultPriority... similar here return virAsprintf(&virLogDefaultOutput, ...); 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. Again use virLogDefaultPriority and "libvirt.log" gets replaced by %s with the 3rd param being fname Promote 'mode_t old_umask;' to the top too Use virLogDefaultPriority - likewise with 3rd param similar in here w/r/t virAsprintf(&virLogDefaultOutput, ...) 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. 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); }

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). 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(a)redhat.com&gt; --- 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 @@ -660,65 +660,24 @@ daemonSetupNetworking(virNetServerPtr srv, static int daemonSetupLoggingDefaults(bool godaemon, bool privileged) { - 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; + 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()) { + char *tmp = virLogGetDefaultOutput(); + virLogSetOutputs(tmp); + VIR_FREE(tmp); + } return 0; - - error: - return -1; } -- 2.5.5

Enable libvirt users to query logging output settings. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- 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; } + +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) { + if (VIR_ALLOC(tmp) < 0) + return -1; + memset(tmp, 0, 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; + + ret->noutputs = noutputs; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + 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) { + if (VIR_STRDUP(tmp_outputs, ret.outputs) < 0) + goto cleanup; + + *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). + * 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); + + 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; }; -- 2.5.5

Enable libvirt users to query logging filter settings. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- 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(-) 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; + } 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; virAdmConnectGetLoggingOutputs; }; -- 2.5.5

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(a)redhat.com&gt; --- 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; - } 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). + * + * 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); + + 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; }; 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(); 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) -- 2.5.5

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 ""). True, I certainly don't have to make the call. 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. Thanks, Erik

Enable libvirt users to modify logging filters of a daemon from outside. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- 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). + * + * 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; virAdmConnectSetLoggingOutputs; }; -- 2.5.5

On Wed, Nov 09, 2016 at 11:26:26AM -0500, John Ferlan wrote: 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. Erik

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(a)redhat.com&gt; --- tools/virt-admin.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) 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 + * ------------------- + */ +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 + * ------------------------- + */ +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} }; -- 2.5.5

On Wed, Nov 09, 2016 at 11:26:31AM -0500, John Ferlan wrote: Shame on me. I'll fix this in the next version right after we reach a consensus on all the patches. 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. Erik