5:19 a.m.
In src/libxl/libxl_conf.c:libxlDriverConfigInit, virLogGetDefaultPriority is used to
specify the (private) loglevel of an external library. This value could be controlled via
"log_level=N" in libvirtd.conf. But doing it that way will affect libvirtd
itself, instead of libxenlight.so as intended. There might be ways to suppress everything
but libxl output via "log_filters=", but it is not clear what the syntax would
be.
I wonder what the preferred way is to specify the desired loglevel for the external
library. It seems a separate configuration setting is required so that each value of XTL_*
from xentoollog.h can be specified.
Olaf
5:44 a.m.
On Wed, Jun 16, 2021 at 12:19:16PM +0200, Olaf Hering wrote:
In src/libxl/libxl_conf.c:libxlDriverConfigInit,
virLogGetDefaultPriority
is used to specify the (private) loglevel of an external library. This
value could be controlled via "log_level=N" in libvirtd.conf. But doing
it that way will affect libvirtd itself, instead of libxenlight.so as
intended. There might be ways to suppress everything but libxl output
via "log_filters=", but it is not clear what the syntax would be.
We support wildcards so probably:
log_filters="3:*"
I wonder what the preferred way is to specify the desired loglevel
for
the external library. It seems a separate configuration setting is
required so that each value of XTL_* from xentoollog.h can be specified.
In general I think the global log level concept in libvirt is flawed,
because libvirt has way too much logging present for it to be viable
to control level globally.
So the right answer is really to use the fine grained logging filters
which control logging per virLogSource object. Usually there is a single
virLogSource per file.
In the case of libxl, it provides callbacks that can be used to
receive the messages and then output them wherever is needed. The
libxl driver does this to write logs into the per-domain log file.
The problem is that the libvirt_vmessage method is filtering messages
based on the 'minLevel' field which is set from virLogGetDefaultPriority
I think the better solution is to filter based on a virLogSource object.
Two options in this respect. Either we can use the existing
VIR_LOG_INIT("libxl.libxl_logger") source from libxl_logger.c
Or we can define a second logging source manually
static virLogSource virLogLibXL = {
.name = "libxl.libxl_library",
.priority = VIR_LOG_ERROR,
.serial = 0,
}
The only complication here is that the 'priority' field is not valid
until we've called virLogVMessage with this virLogSource instance.
If we're reusing the existing virLogSource instance from libxl_logger.c
that's ok, as we have a VIR_DEBUG call that will trigger update of the
'priority' field.
If we're defining a custom virLogSource instance, then we need to make
the virLogSourceUpdate() method public from virlog.c, so that you can
call it to update 'priority'. eg libvirt_vmessage needs todo
if (source->serial < virLogFiltersSerial)
virLogSourceUpdate(source);
Anyway if you used a virLogSource object, then turning on debugging
exclusively for libxl library messages would be as simple as
log_filters="1:libxl.libxl_library"
without affecting the global libvirt logging behaviour
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
9:39 a.m.
Thanks for your reply, I had no time to look at this until now.
Am Wed, 16 Jun 2021 11:44:10 +0100
schrieb Daniel P. Berrangé <berrange(a)redhat.com>:
Anyway if you used a virLogSource object, then turning on debugging
exclusively for libxl library messages would be as simple as
log_filters="1:libxl.libxl_library"
without affecting the global libvirt logging behaviour
It seems log_filters= does not allow arbitrary numbers.
virLogParseFilter would just allow values from 1 to 4.
Such extra logfilter needs to handle a xentoollog_level, values from 1 to 9 (>XTL_NONE
and <XTL_NUM_LEVELS).
How should this be expressed in a config setting?
Perhaps just go the easy route and obtain a value from environment?
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1829,7 +1829,7 @@ libxlDriverConfigInit(libxlDriverConfig *cfg)
return -1;
}
- cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
+ cfg->logger = libxlLoggerNew(cfg->logDir);
if (!cfg->logger) {
VIR_ERROR(_("cannot create logger for libxenlight, disabling
driver"));
return -1;
--- a/src/libxl/libxl_logger.c
+++ b/src/libxl/libxl_logger.c
@@ -129,25 +129,33 @@ libvirt_destroy(xentoollog_logger *logger_in)
libxlLogger *
-libxlLoggerNew(const char *logDir, virLogPriority minLevel)
+libxlLoggerNew(const char *logDir)
{
xentoollog_logger_libvirt logger;
g_autofree char *path = NULL;
-
- switch (minLevel) {
- case VIR_LOG_DEBUG:
- logger.minLevel = XTL_DEBUG;
- break;
- case VIR_LOG_INFO:
- logger.minLevel = XTL_INFO;
- break;
- case VIR_LOG_WARN:
- logger.minLevel = XTL_WARN;
- break;
- case VIR_LOG_ERROR:
- logger.minLevel = XTL_ERROR;
- break;
+ char *xenlight_minlevel = getenv("xenlight_minlevel");
+ int minlevel;
+
+ if (!(xenlight_minlevel && *xenlight_minlevel &&
+ virStrToLong_i(xenlight_minlevel, NULL, 10, &minlevel) >= 0 &&
+ minlevel > XTL_NONE && minlevel < XTL_NUM_LEVELS)) {
+ minlevel = virLogGetDefaultPriority();
+ switch (minlevel) {
+ case VIR_LOG_DEBUG:
+ minlevel = XTL_DEBUG;
+ break;
+ case VIR_LOG_INFO:
+ minlevel = XTL_INFO;
+ break;
+ case VIR_LOG_WARN:
+ minlevel = XTL_WARN;
+ break;
+ case VIR_LOG_ERROR:
+ minlevel = XTL_ERROR;
+ break;
+ }
}
+ logger.minLevel = minlevel;
logger.logDir = logDir;
if ((logger.files = virHashNew(libxlLoggerFileFree)) == NULL)
--- a/src/libxl/libxl_logger.h
+++ b/src/libxl/libxl_logger.h
@@ -24,8 +24,7 @@
typedef struct xentoollog_logger_libvirt libxlLogger;
-libxlLogger *libxlLoggerNew(const char *logDir,
- virLogPriority minLevel);
+libxlLogger *libxlLoggerNew(const char *logDir);
void libxlLoggerFree(libxlLogger *logger);
void libxlLoggerOpenFile(libxlLogger *logger, int id, const char *name,
Olaf
10:07 a.m.
On Thu, Jul 22, 2021 at 04:39:24PM +0200, Olaf Hering wrote:
Thanks for your reply, I had no time to look at this until now.
Am Wed, 16 Jun 2021 11:44:10 +0100
schrieb Daniel P. Berrangé <berrange(a)redhat.com>:
> Anyway if you used a virLogSource object, then turning on debugging
> exclusively for libxl library messages would be as simple as
>
> log_filters="1:libxl.libxl_library"
>
> without affecting the global libvirt logging behaviour
It seems log_filters= does not allow arbitrary numbers.
virLogParseFilter would just allow values from 1 to 4.
Such extra logfilter needs to handle a xentoollog_level, values from 1 to 9 (>XTL_NONE
and <XTL_NUM_LEVELS).
How should this be expressed in a config setting?
Do we actually need to care about every single level, as opposed to
just doing a sparse mapping where we squash several xen levels into
one libvirt level, as the code below shows ?
Perhaps just go the easy route and obtain a value from environment?
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1829,7 +1829,7 @@ libxlDriverConfigInit(libxlDriverConfig *cfg)
return -1;
}
- cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
+ cfg->logger = libxlLoggerNew(cfg->logDir);
if (!cfg->logger) {
VIR_ERROR(_("cannot create logger for libxenlight, disabling
driver"));
return -1;
--- a/src/libxl/libxl_logger.c
+++ b/src/libxl/libxl_logger.c
@@ -129,25 +129,33 @@ libvirt_destroy(xentoollog_logger *logger_in)
libxlLogger *
-libxlLoggerNew(const char *logDir, virLogPriority minLevel)
+libxlLoggerNew(const char *logDir)
{
xentoollog_logger_libvirt logger;
g_autofree char *path = NULL;
-
- switch (minLevel) {
- case VIR_LOG_DEBUG:
- logger.minLevel = XTL_DEBUG;
- break;
- case VIR_LOG_INFO:
- logger.minLevel = XTL_INFO;
- break;
- case VIR_LOG_WARN:
- logger.minLevel = XTL_WARN;
- break;
- case VIR_LOG_ERROR:
- logger.minLevel = XTL_ERROR;
- break;
+ char *xenlight_minlevel = getenv("xenlight_minlevel");
+ int minlevel;
+
+ if (!(xenlight_minlevel && *xenlight_minlevel &&
+ virStrToLong_i(xenlight_minlevel, NULL, 10, &minlevel) >= 0 &&
+ minlevel > XTL_NONE && minlevel < XTL_NUM_LEVELS)) {
+ minlevel = virLogGetDefaultPriority();
+ switch (minlevel) {
+ case VIR_LOG_DEBUG:
+ minlevel = XTL_DEBUG;
+ break;
+ case VIR_LOG_INFO:
+ minlevel = XTL_INFO;
+ break;
+ case VIR_LOG_WARN:
+ minlevel = XTL_WARN;
+ break;
+ case VIR_LOG_ERROR:
+ minlevel = XTL_ERROR;
+ break;
+ }
}
+ logger.minLevel = minlevel;
logger.logDir = logDir;
if ((logger.files = virHashNew(libxlLoggerFileFree)) == NULL)
--- a/src/libxl/libxl_logger.h
+++ b/src/libxl/libxl_logger.h
@@ -24,8 +24,7 @@
typedef struct xentoollog_logger_libvirt libxlLogger;
-libxlLogger *libxlLoggerNew(const char *logDir,
- virLogPriority minLevel);
+libxlLogger *libxlLoggerNew(const char *logDir);
void libxlLoggerFree(libxlLogger *logger);
void libxlLoggerOpenFile(libxlLogger *logger, int id, const char *name,
Olaf
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:17 a.m.
Am Thu, 22 Jul 2021 16:07:15 +0100
schrieb Daniel P. Berrangé <berrange(a)redhat.com>:
Do we actually need to care about every single level, as opposed to
just doing a sparse mapping where we squash several xen levels into
one libvirt level, as the code below shows ?
xc_private.h uses XTL_INFO, XTL_DETAIL, XTL_DEBUG and XTL_ERROR in the wrappers.
I think in the end all of the values can be used once execution enters libxenlight.so.
The code I changed had to map the few libvirt values somehow to the XTL values.
Olaf
5:26 p.m.
On 7/22/21 11:28 AM, Olaf Hering wrote:
Am Thu, 22 Jul 2021 17:17:11 +0200
schrieb Olaf Hering <olaf(a)aepfle.de>:
> xc_private.h uses XTL_INFO, XTL_DETAIL, XTL_DEBUG and XTL_ERROR in the wrappers.
I wonder if the code should map VIR_LOG_INFO to XTL_DETAIL to enable DPRINTF.
Does the output of DPRINTF pass the "info" smell test? :-) Or is it only useful
for debugging? INFO vs DEBUG can be a bit gray IMO, but reviewing the use of
VIR_INFO vs VIR_DEBUG throughout the code might help decide what side of the
fence DPRINTF lies.
Jim
3:17 a.m.
Am Fri, 23 Jul 2021 16:26:10 -0600
schrieb Jim Fehlig <jfehlig(a)suse.com>:
Does the output of DPRINTF pass the "info" smell test? :-)
Or is it only useful
for debugging? INFO vs DEBUG can be a bit gray IMO, but reviewing the use of
VIR_INFO vs VIR_DEBUG throughout the code might help decide what side of the
fence DPRINTF lies.
I think it is wrong to use internal libvirt values for internal libxentoollog values.
There has to be a separate configuration setting to specify the various xentoollog_level
values. It is just the question where to put such setting, and how to name it.
I guess no other used library provides similar internal logging, this requirement is
unique to libxenlight.so.
Olaf
5:30 a.m.
This question is still open:
where should the libxl specific setting be specified?
Olaf
Am Sun, 25 Jul 2021 10:17:49 +0200
schrieb Olaf Hering <olaf(a)aepfle.de>:
I think it is wrong to use internal libvirt values for internal
libxentoollog values.
There has to be a separate configuration setting to specify the various xentoollog_level
values. It is just the question where to put such setting, and how to name it.
I guess no other used library provides similar internal logging, this requirement is
unique to libxenlight.so.
2:25 p.m.
On 10/11/21 04:30, Olaf Hering wrote:
This question is still open:
where should the libxl specific setting be specified?
Sorry for not responding to your earlier mail.
Am Sun, 25 Jul 2021 10:17:49 +0200
schrieb Olaf Hering <olaf(a)aepfle.de>:
> I think it is wrong to use internal libvirt values for internal libxentoollog values.
What is gained over the sparse mapping? Are values not covered actually useful
in practice? Also, how are the values specified when using the xl tool? A quick
peek at the code seems to imply using more 'v' options. E.g. 'xl -vvvv
...'.
> There has to be a separate configuration setting to specify the
various xentoollog_level values. It is just the question where to put such setting, and
how to name it.
If such a setting is actually needed, then the proper place would be
/etc/libvirt/libxl.conf. And IMO we should avoid creating a new name that could
potentially confuse users. If 'xentoollog_level' is the common jargon in xen, we
should stick with the same name.
> I guess no other used library provides similar internal logging,
this requirement is unique to libxenlight.so.
Good question, but I think you are right. It is unique to libxenlight.
Jim
3:38 p.m.
Am Mon, 11 Oct 2021 13:25:20 -0600
schrieb Jim Fehlig <jfehlig(a)suse.com>:
>> I think it is wrong to use internal libvirt values for
internal libxentoollog values.
What is gained over the sparse mapping? Are values not covered actually useful
in practice? Also, how are the values specified when using the xl tool? A quick
peek at the code seems to imply using more 'v' options. E.g. 'xl -vvvv
...'.
Since libvirt has no control about what libxentoollog values the authors
use, it should provide a simple way to request a specific loglevel.
xl decided to use the number of v's for it.
If such a setting is actually needed, then the proper place would be
/etc/libvirt/libxl.conf. And IMO we should avoid creating a new name that could
potentially confuse users. If 'xentoollog_level' is the common jargon in xen, we
should stick with the same name.
If that is an acceptable approach I will try to prepare a patch.
Olaf
4:20 p.m.
On 10/11/21 14:38, Olaf Hering wrote:
Am Mon, 11 Oct 2021 13:25:20 -0600
schrieb Jim Fehlig <jfehlig(a)suse.com>:
>>> I think it is wrong to use internal libvirt values for internal libxentoollog
values.
> What is gained over the sparse mapping? Are values not covered actually useful
> in practice? Also, how are the values specified when using the xl tool? A quick
> peek at the code seems to imply using more 'v' options. E.g. 'xl -vvvv
...'.
Since libvirt has no control about what libxentoollog values the authors
use, it should provide a simple way to request a specific loglevel.
xl decided to use the number of v's for it.
How does one correlate a specific log level to the correct number of 'v'? :-)
IMO the sparse mapping we currently have is more intuitive.
> If such a setting is actually needed, then the proper place would
be
> /etc/libvirt/libxl.conf. And IMO we should avoid creating a new name that could
> potentially confuse users. If 'xentoollog_level' is the common jargon in xen,
we
> should stick with the same name.
If that is an acceptable approach I will try to prepare a patch.
I must admit I'm not fond of another log level knob. For many classes of bugs,
users would then need to tweak libvirtd.conf and libxl.conf to collect
appropriate debug info. That said, there is prior art. Going back to your
snipped question
> I guess no other used library provides similar internal logging,
this
requirement is unique to libxenlight.so.
Sorry for not looking closer earlier, but the qemu driver has a similar setting
in qemu.conf for the gluster libgfapi log level
https://gitlab.com/libvirt/libvirt/-/commit/a944bd925902d9ecce81e08900ad6...
Jim
1100
days inactive
1217
days old
11 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Jim Fehlig -
Olaf Hering