10:21 a.m.
On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
>
>
> On 11/25/2016 08:12 AM, Erik Skultety wrote:
>> Finally, now that all APIs have been introduced, wire them up to virt-admin
>> and introduce daemon-log-outputs and daemon-log-filters commands.
>>
>> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
>> ---
>> tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 210 insertions(+)
>>
>
> Now it *is* the coffee - should there be any concerns along the way over
> escaping characters? I think this is the only place where we
> read/print, but via the API if someone passes a string - do we need to
> worry about that? Yes, I know - not the normal thing, but you know
> someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have
to create a logfile with that name on the filesystem. When we retrieve it, I
think we should print it as we created it - garbage. The other option we have
is to strip all the control characters and print the rest which in the worst
case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape
the 'unprintable' chars when we Format the output.
The first thing that came to mind that might be close to what's being
done here was domain interface script, where when we print out the name
we Escape whatever is in script path.
Looking at the src/util/virtconf.c for VIR_CONF_STRING and
virConfParseString would seem to imply to me the ability to read escaped
characters.
The difference here is that you're now formatting the name; whereas,
previously the formatting was entirely left to whomever edited
/etc/libvirt/libvirtd.conf.
In the long run, recent memory says whenever there's a user provided
character string - the review comments have always been be sure to
Escape it.
Searching for Escape in tools/*.c found a couple more "output" files.
>
>
>> diff --git a/tools/virt-admin.c b/tools/virt-admin.c
>> index 70b0e51..0b9945a 100644
>> --- a/tools/virt-admin.c
>> +++ b/tools/virt-admin.c
>> @@ -971,6 +971,114 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd)
>> goto cleanup;
>> }
>>
>> +/* --------------------------
>> + * Command daemon-log-filters
>> + * --------------------------
>> + */
>> +static const vshCmdInfo info_daemon_log_filters[] = {
>> + {.name = "help",
>> + .data = N_("fetch or set the currently defined set of logging filters
on "
>> + "daemon")
>> + },
>> + {.name = "desc",
>> + .data = N_("Depending on whether run with or without options, the
command "
>> + "fetches or redefines the existing active set of filters on
"
>> + "daemon.")
>> + },
>> + {.name = NULL}
>> +};
>> +
>> +static const vshCmdOptDef opts_daemon_log_filters[] = {
>> + {.name = "filters",
>> + .type = VSH_OT_STRING,
>> + .help = N_("redefine the existing set of logging filters"),
>> + .flags = VSH_OFLAG_EMPTY_OK
>> + },
>> + {.name = NULL}
>> +};
>> +
>> +static bool
>> +cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd)
>> +{
>> + int nfilters;
>> + char *filters = NULL;
>> + vshAdmControlPtr priv = ctl->privData;
>> +
>> + if (vshCommandOptBool(cmd, "filters")) {
>> + if ((vshCommandOptStringReq(ctl, cmd, "filters",
>> + (const char **) &filters) < 0 ||
>> + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0))
{
>> + vshError(ctl, _("Unable to change daemon logging
settings"));
>> + return false;
>> + }
>> + } else {
>> + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn,
>> + &filters, 0)) < 0)
{
>> + vshError(ctl, _("Unable to get daemon logging filters
information"));
>> + return false;
>> + }
>> +
>> + vshPrintExtra(ctl, " %-15s", _("Logging filters:
"));
>> + vshPrint(ctl, "%s\n", filters ? filters : "");
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/* --------------------------
>> + * Command daemon-log-outputs
>> + * --------------------------
>> + */
>> +static const vshCmdInfo info_daemon_log_outputs[] = {
>> + {.name = "help",
>> + .data = N_("fetch or set the currently defined set of logging outputs
on "
>> + "daemon")
>> + },
>> + {.name = "desc",
>> + .data = N_("Depending on whether run with or without options, the
command "
>> + "fetches or redefines the existing active set of outputs on
"
>> + "daemon.")
>> + },
>> + {.name = NULL}
>> +};
>> +
>> +static const vshCmdOptDef opts_daemon_log_outputs[] = {
>> + {.name = "outputs",
>> + .type = VSH_OT_STRING,
>> + .help = N_("redefine the existing set of logging outputs"),
>> + .flags = VSH_OFLAG_EMPTY_OK
>> + },
>> + {.name = NULL}
>> +};
>> +
>> +static bool
>> +cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd)
>> +{
>> + int noutputs;
>> + char *outputs = NULL;
>> + vshAdmControlPtr priv = ctl->privData;
>> +
>> + if (vshCommandOptBool(cmd, "outputs")) {
>> + if ((vshCommandOptStringReq(ctl, cmd, "outputs",
>> + (const char **) &outputs) < 0 ||
>> + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0))
{
>> + vshError(ctl, _("Unable to change daemon logging
settings"));
>> + return false;
>> + }
>> + } else {
>> + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn,
>> + &outputs, 0)) < 0)
{
>> + vshError(ctl, _("Unable to get daemon logging filters
information"));
>> + return false;
>> + }
>> +
>> + vshPrintExtra(ctl, " %-15s", _("Logging outputs:
"));
>> + vshPrint(ctl, "%s\n", outputs ? outputs : "");
>> + }
>> +
>> + return true;
>> +}
>> +
>> static void *
>> vshAdmConnectionHandler(vshControl *ctl)
>> {
>> @@ -1341,6 +1449,18 @@ static const vshCmdDef managementCmds[] = {
>> .info = info_srv_clients_set,
>> .flags = 0
>> },
>> + {.name = "daemon-log-filters",
>> + .handler = cmdDaemonLogFilters,
>> + .opts = opts_daemon_log_filters,
>> + .info = info_daemon_log_filters,
>> + .flags = 0
>> + },
>> + {.name = "daemon-log-outputs",
>> + .handler = cmdDaemonLogOutputs,
>> + .opts = opts_daemon_log_outputs,
>> + .info = info_daemon_log_outputs,
>> + .flags = 0
>> + },
>> {.name = NULL}
>> };
>>
>> diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod
>> index 443730e..d3f1a35 100644
>> --- a/tools/virt-admin.pod
>> +++ b/tools/virt-admin.pod
>
> All of this is way off - no such command as daemon-log-info or
> daemon-log-define.
>
> I think you should reference /etc/libvirt/libvirtd.conf in order to
> point at the place which describes the format.
Not sure what you mean, should I just add a reference or replace the sections
explaining the format with the reference completely?
I was think more along the lines of adding the reference indicating that
log filters are described in detail in the /etc/libvirt/libvirtd.conf
file. Similarly for log_outputs. That avoids repeating descriptions
here and there.
John
Erik
>
> Although I trust you know what should be there so...
>
> ACK once you clean this up.
>
> John
>
>> @@ -155,6 +155,96 @@ change its internal configuration.
>> Lists all manageable servers contained within the daemon the client is
>> currently connected to.
>>
>> +=item B<daemon-log-info> [I<--outputs> B<string>]
[I<--filters> B<string>]
>> +
>> +Print the currently defined set of logging outputs and/or filters.
>> +
>> +=over 4
>> +
>> +=item I<--outputs>
>> +
>> +Print the currently defined set of logging outputs
>> +
>> +=item I<--filters>
>> +
>> +Print the currently defined set of logging filters
>> +
>> +=back
>> +
>> +=item B<daemon-log-define>
>> +
>> +Redefine the currently defined sets of logging outputs and/or filters with
>> +completely new ones.
>> +
>> +=over 4
>> +
>> +=item I<--outputs>
>> +
>> +Replaces the current set of logging outputs with a new one where multiple
>> +outputs are delimited by space and each output must be of the following form:
>> +
>> +I<< X:<destination>:<auxiliary_data> >> where
>> +
>> +=over 4
>> +
>> +=item * I<X> denotes the minimal debug level:
>> +
>> +=over 4
>> +
>> +=item * 1: DEBUG
>> +
>> +=item * 2: INFO
>> +
>> +=item * 3: WARNING
>> +
>> +=item * 4: ERROR
>> +
>> +=back
>> +
>> +=item * I<< <destination> >> is one of I<stderr>,
I<file>, I<syslog>, or I<journald>
>> +
>> +=item * I<< <auxiliary_data> >> is only required for the
I<file> and I<syslog>
>> +destination types and shall therefore contain either a path to a file or a
message tag
>> +respectively, see the B<Examples> section below.
>> +
>> +=back
>> +
>> +=item I<--filters>
>> +
>> +Replaces the current set of logging filters with a new one where multiple
>> +filters are delimited by space and each filter must be of the following form:
>> +
>> +I<< X:<match_string> >> where
>> +
>> +=over 4
>> +
>> +=item * I<X> denotes the minimal debug level the same way as for
I<--outputs>
>> +
>> +=item * I<< <match_string> >> references either a specific
module in libvirt's
>> +source tree or the whole directory in the source tree, thus affecting all
>> +modules in the directory
>> +
>> +=back
>> +
>> +=back
>> +
>> +B<Examples>
>> + To replace the current logging output with one that writes to a file while
>> + logging logging errors only, the following could be used:
>> +
>> + $ virt-admin daemon-log-define --outputs
"4:file:<path_to_the_file>"
>> +
>> + To define multiple outputs at once:
>> +
>> + $ virt-admin daemon-log-define --outputs "4:stderr
2:syslog:<tag>"
>> +
>> + To define a filter which suppresses all e.g. 'virObjectUnref' DEBUG
>> + messages, use the following (note the '.' symbol which can be used
to more
>> + fine-grained filters tailored to specific modules, in contrast, to affect
>> + a whole directory containing several modules it would become
"4:util"):
>> +
>> + $ virt-admin daemon-log-define --filters "4:util.object"
>> +
>> =back
>>
>> =head1 SERVER COMMANDS
>>