[PATCH 0/6] util: log infor to help troubleshoot file/socket open failures
The first couple patches are just related cleanups I noticed while doing the others, then a couple patches to simplify adding more lines to the bannder printed at the top of the log, and then finally the last couple patches add some info to the banner at the top of the log that would be generally useful when trying to understand why some random file or socket failed to be created or opened. Ideas on what else to add (or what to remove, or how to better format it) are welcome! Laine Stump (6): util: log the name of the log directory that couldn't be created util: consistently use typedef virLogMetadata util: eliminate duplicate code in virLogVMessage util: make it easier to add lines to the log "init banner" util: add info to log banner about uid and user environment util: add info about g_get_user_*_dir directories to log banner src/util/virlog.c | 231 ++++++++++++++++++++++++++++++---------------- src/util/virlog.h | 4 +- tests/testutils.c | 2 +- 3 files changed, 155 insertions(+), 82 deletions(-) -- 2.53.0
From: Laine Stump <laine@redhat.com> The message previously just said "Could not create log directory", but didn't provide the name of the directory, which could be helpful in determine why the failure occured. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index c24dfa83c4..9f53c72975 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -178,7 +178,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) old_umask = umask(077); if (g_mkdir_with_parents(logdir, 0777) < 0) { umask(old_umask); - virReportSystemError(errno, "%s", _("Could not create log directory")); + virReportSystemError(errno, _("Could not create log directory '%1$s'"), logdir); return -1; } umask(old_umask); -- 2.53.0
On Mon, Apr 06, 2026 at 23:29:15 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
The message previously just said "Could not create log directory", but didn't provide the name of the directory, which could be helpful in determine why the failure occured.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Laine Stump <laine@redhat.com> For some reason there were some uses of this struct where "struct _virLogMetadata" was used instead of just using the typedef "virLogMetadata" (they are both defined in the same file - virlog.h). Possibly at one point the struct was in virlog.c and outsiders could only see it as an opaque object, but even if that was the case, there are already cases of the typedef being used outside of virlog.c, and constinuing to use "struct _virLogMetadata" in some places both looks too much K&R 1st edition and might incorrectly imply to someone that there *is* data abstraction/hiding going on when there really isn't. So let's just always use plain virLogMetadata. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 12 ++++++------ src/util/virlog.h | 4 ++-- tests/testutils.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 9f53c72975..30cb68fe7d 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -116,7 +116,7 @@ static void virLogOutputToFd(virLogSource *src, int linenr, const char *funcname, const char *timestamp, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *rawstr, const char *str, void *data); @@ -500,7 +500,7 @@ virLogVMessage(virLogSource *source, const char *filename, int linenr, const char *funcname, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *fmt, va_list vargs) { @@ -627,7 +627,7 @@ virLogMessage(virLogSource *source, const char *filename, int linenr, const char *funcname, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *fmt, ...) { va_list ap; @@ -646,7 +646,7 @@ virLogOutputToFd(virLogSource *source G_GNUC_UNUSED, int linenr G_GNUC_UNUSED, const char *funcname G_GNUC_UNUSED, const char *timestamp, - struct _virLogMetadata *metadata G_GNUC_UNUSED, + virLogMetadata *metadata G_GNUC_UNUSED, const char *rawstr G_GNUC_UNUSED, const char *str, void *data) @@ -746,7 +746,7 @@ virLogOutputToSyslog(virLogSource *source G_GNUC_UNUSED, int linenr G_GNUC_UNUSED, const char *funcname G_GNUC_UNUSED, const char *timestamp G_GNUC_UNUSED, - struct _virLogMetadata *metadata G_GNUC_UNUSED, + virLogMetadata *metadata G_GNUC_UNUSED, const char *rawstr G_GNUC_UNUSED, const char *str, void *data G_GNUC_UNUSED) @@ -890,7 +890,7 @@ virLogOutputToJournald(virLogSource *source, int linenr, const char *funcname, const char *timestamp G_GNUC_UNUSED, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *rawstr, const char *str G_GNUC_UNUSED, void *data) diff --git a/src/util/virlog.h b/src/util/virlog.h index 4f755c543b..77ba40b2e4 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -127,7 +127,7 @@ typedef void (*virLogOutputFunc) (virLogSource *src, int linenr, const char *funcname, const char *timestamp, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *rawstr, const char *str, void *data); @@ -170,7 +170,7 @@ void virLogMessage(virLogSource *source, const char *filename, int linenr, const char *funcname, - struct _virLogMetadata *metadata, + virLogMetadata *metadata, const char *fmt, ...) G_GNUC_PRINTF(7, 8); bool virLogProbablyLogMessage(const char *str); diff --git a/tests/testutils.c b/tests/testutils.c index 14e5c56fcf..35571fb2ad 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -680,7 +680,7 @@ virtTestLogOutput(virLogSource *source G_GNUC_UNUSED, int lineno G_GNUC_UNUSED, const char *funcname G_GNUC_UNUSED, const char *timestamp, - struct _virLogMetadata *metadata G_GNUC_UNUSED, + virLogMetadata *metadata G_GNUC_UNUSED, const char *rawstr G_GNUC_UNUSED, const char *str, void *data) -- 2.53.0
On Mon, Apr 06, 2026 at 23:29:16 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
For some reason there were some uses of this struct where "struct _virLogMetadata" was used instead of just using the typedef "virLogMetadata" (they are both defined in the same file - virlog.h). Possibly at one point the struct was in virlog.c and outsiders could only see it as an opaque object, but even if that was the case, there are already cases of the typedef being used outside of virlog.c, and constinuing to use "struct _virLogMetadata" in some places both looks too much K&R 1st edition and might incorrectly imply to someone that there *is* data abstraction/hiding going on when there really isn't. So let's just always use plain virLogMetadata.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 12 ++++++------ src/util/virlog.h | 4 ++-- tests/testutils.c | 2 +-
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Laine Stump <laine@redhat.com> The same several lines were repeated, once in a loop iterating through all log targets, and again to output to stderr when there are no log targets specified. This just moves those lines into a helper function, making it easier and less error prone to add additional info the the banner that is logged each time a daemon starts logging. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 127 ++++++++++++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 48 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 30cb68fe7d..7a9b1e75d5 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -479,6 +479,75 @@ virLogSourceUpdate(virLogSource *source) } +/** + * virLogToOneTarget: + * + * (these first several args are coming directly from the args of + * virLogVMessage() - you can find their description there) + * + * @source, @priority, @filename, @linenr, @funcname, @metadata: + * + * (the next 3 are created once during each call to virLogMMessage() and reused + * for each target) + * + * @timestamp: cached (during this one log to multiple targets) raw time + * @str: the log message formatted from what appears in the VIR_*() + or virReport*() call + * @msg: the formatted log message with function name, line number, and + * priority added + * + * @outputFunc: pointer to function to call to output the data + * @data: private data used by @outputFunc (e.g. fd to write to) + * @needInit: pointer to bool that gets set to false once the + * once-per-daemon-run init message has been sent to this target + * + * If needInit is true, construct the strings to send the "init" + * message (a banner with software version, etc) to the log target + * using @outputFunc and set @needInit to false. Then send the current + * log message to the target (described by the other args) using + * @outputFunc. + */ +static void +virLogToOneTarget(virLogSource *source, + virLogPriority priority, + const char *filename, + int linenr, + const char *funcname, + virLogMetadata *metadata, + const char *timestamp, + const char *str, + const char *msg, + virLogOutputFunc outputFunc, + void *data, + bool *needInit) +{ + if (needInit) { + const char *rawinitmsg; + char *hoststr = NULL; + char *initmsg = NULL; + + virLogVersionString(&rawinitmsg, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, rawinitmsg, initmsg, + data); + VIR_FREE(initmsg); + + virLogHostnameString(&hoststr, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, hoststr, initmsg, + data); + VIR_FREE(hoststr); + VIR_FREE(initmsg); + needInit = false; + } + + outputFunc(source, priority, filename, linenr, funcname, + timestamp, metadata, str, msg, data); +} + + /** * virLogVMessage: * @source: where is that message coming from @@ -548,57 +617,19 @@ virLogVMessage(virLogSource *source, */ for (i = 0; i < virLogNbOutputs; i++) { if (priority >= virLogOutputs[i]->priority) { - if (virLogOutputs[i]->logInitMessage) { - const char *rawinitmsg; - char *hoststr = NULL; - char *initmsg = NULL; - virLogVersionString(&rawinitmsg, &initmsg); - virLogOutputs[i]->f(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, rawinitmsg, initmsg, - virLogOutputs[i]->data); - VIR_FREE(initmsg); - - virLogHostnameString(&hoststr, &initmsg); - virLogOutputs[i]->f(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, hoststr, initmsg, - virLogOutputs[i]->data); - VIR_FREE(hoststr); - VIR_FREE(initmsg); - virLogOutputs[i]->logInitMessage = false; - } - virLogOutputs[i]->f(source, priority, - filename, linenr, funcname, - timestamp, metadata, - str, msg, virLogOutputs[i]->data); + virLogToOneTarget(source, priority, filename, linenr, funcname, metadata, + timestamp, str, msg, + virLogOutputs[i]->f, + virLogOutputs[i]->data, + &virLogOutputs[i]->logInitMessage); } } if (virLogNbOutputs == 0) { - if (logInitMessageStderr) { - const char *rawinitmsg; - char *hoststr = NULL; - char *initmsg = NULL; - virLogVersionString(&rawinitmsg, &initmsg); - virLogOutputToFd(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, rawinitmsg, initmsg, - (void *) STDERR_FILENO); - VIR_FREE(initmsg); - - virLogHostnameString(&hoststr, &initmsg); - virLogOutputToFd(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, hoststr, initmsg, - (void *) STDERR_FILENO); - VIR_FREE(hoststr); - VIR_FREE(initmsg); - logInitMessageStderr = false; - } - virLogOutputToFd(source, priority, - filename, linenr, funcname, - timestamp, metadata, - str, msg, (void *) STDERR_FILENO); + virLogToOneTarget(source, priority, filename, linenr, funcname, metadata, + timestamp, str, msg, + virLogOutputToFd, + (void *) STDERR_FILENO, + &logInitMessageStderr); } virLogUnlock(); -- 2.53.0
On Mon, Apr 06, 2026 at 23:29:17 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
The same several lines were repeated, once in a loop iterating through all log targets, and again to output to stderr when there are no log targets specified. This just moves those lines into a helper function, making it easier and less error prone to add additional info the the banner that is logged each time a daemon starts logging.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 127 ++++++++++++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 48 deletions(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 30cb68fe7d..7a9b1e75d5 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -479,6 +479,75 @@ virLogSourceUpdate(virLogSource *source) }
+/** + * virLogToOneTarget: + * + * (these first several args are coming directly from the args of + * virLogVMessage() - you can find their description there) + * + * @source, @priority, @filename, @linenr, @funcname, @metadata: + * + * (the next 3 are created once during each call to virLogMMessage() and reused + * for each target) + * + * @timestamp: cached (during this one log to multiple targets) raw time + * @str: the log message formatted from what appears in the VIR_*() + or virReport*() call + * @msg: the formatted log message with function name, line number, and + * priority added + * + * @outputFunc: pointer to function to call to output the data + * @data: private data used by @outputFunc (e.g. fd to write to) + * @needInit: pointer to bool that gets set to false once the + * once-per-daemon-run init message has been sent to this target + * + * If needInit is true, construct the strings to send the "init" + * message (a banner with software version, etc) to the log target + * using @outputFunc and set @needInit to false. Then send the current + * log message to the target (described by the other args) using + * @outputFunc. + */ +static void +virLogToOneTarget(virLogSource *source, + virLogPriority priority, + const char *filename, + int linenr, + const char *funcname, + virLogMetadata *metadata, + const char *timestamp, + const char *str, + const char *msg, + virLogOutputFunc outputFunc, + void *data, + bool *needInit) +{ + if (needInit) { + const char *rawinitmsg; + char *hoststr = NULL; + char *initmsg = NULL; + + virLogVersionString(&rawinitmsg, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, rawinitmsg, initmsg, + data); + VIR_FREE(initmsg); + + virLogHostnameString(&hoststr, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, hoststr, initmsg, + data); + VIR_FREE(hoststr); + VIR_FREE(initmsg); + needInit = false;
I understand that this is a code move, but consider converting the strings to g_autofree. It'll require one extra temporary variable but would IMO be cleaner.
+ } + + outputFunc(source, priority, filename, linenr, funcname, + timestamp, metadata, str, msg, data); +} +
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Apr 06, 2026 at 23:29:17 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
The same several lines were repeated, once in a loop iterating through all log targets, and again to output to stderr when there are no log targets specified. This just moves those lines into a helper function, making it easier and less error prone to add additional info the the banner that is logged each time a daemon starts logging.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 127 ++++++++++++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 48 deletions(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 30cb68fe7d..7a9b1e75d5 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -479,6 +479,75 @@ virLogSourceUpdate(virLogSource *source) }
+/** + * virLogToOneTarget: + * + * (these first several args are coming directly from the args of + * virLogVMessage() - you can find their description there) + * + * @source, @priority, @filename, @linenr, @funcname, @metadata: + * + * (the next 3 are created once during each call to virLogMMessage() and reused + * for each target) + * + * @timestamp: cached (during this one log to multiple targets) raw time + * @str: the log message formatted from what appears in the VIR_*() + or virReport*() call + * @msg: the formatted log message with function name, line number, and + * priority added + * + * @outputFunc: pointer to function to call to output the data + * @data: private data used by @outputFunc (e.g. fd to write to) + * @needInit: pointer to bool that gets set to false once the + * once-per-daemon-run init message has been sent to this target + * + * If needInit is true, construct the strings to send the "init" + * message (a banner with software version, etc) to the log target + * using @outputFunc and set @needInit to false. Then send the current + * log message to the target (described by the other args) using + * @outputFunc. + */ +static void +virLogToOneTarget(virLogSource *source, + virLogPriority priority, + const char *filename, + int linenr, + const char *funcname, + virLogMetadata *metadata, + const char *timestamp, + const char *str, + const char *msg, + virLogOutputFunc outputFunc, + void *data, + bool *needInit) +{ + if (needInit) {
This needs to be *needInit ...
+ const char *rawinitmsg; + char *hoststr = NULL; + char *initmsg = NULL; + + virLogVersionString(&rawinitmsg, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, rawinitmsg, initmsg, + data); + VIR_FREE(initmsg); + + virLogHostnameString(&hoststr, &initmsg); + outputFunc(&virLogSelf, VIR_LOG_INFO, + __FILE__, __LINE__, __func__, + timestamp, NULL, hoststr, initmsg, + data); + VIR_FREE(hoststr); + VIR_FREE(initmsg); + needInit = false;
... and this *needInit = false; Since pointers and literals evaluating to 0 actually combine without warnings in C this compiles cleanly but obviously doesn't work as expected (prints the headers along with every debug).
On Mon, Apr 06, 2026 at 11:29:17PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
The same several lines were repeated, once in a loop iterating through all log targets, and again to output to stderr when there are no log targets specified. This just moves those lines into a helper function, making it easier and less error prone to add additional info the the
s/the the/to the/
banner that is logged each time a daemon starts logging.
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
From: Laine Stump <laine@redhat.com> The same thing happens for each line of the log banner: 1) A helper function is called that a) creates a "raw" string (just the desired info, e.g. version string) and b) calls virLogFormatString() for create a "cooked" version of the string (containing thread-id and log priority) 2) the outputFunc for the target is called with strings (a) and (b) By making a helper that does (1b) & (2), we can further reduce the amount of redundant code that needs to be written to add another line to the banner - now all we need to do is: 1) create the raw string 2) call the helper, sending it the raw string Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 81 +++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 7a9b1e75d5..4e95af047b 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -433,30 +433,6 @@ virLogFormatString(char **msg, } -static void -virLogVersionString(const char **rawmsg, - char **msg) -{ - *rawmsg = VIR_LOG_VERSION_STRING; - virLogFormatString(msg, 0, NULL, VIR_LOG_INFO, VIR_LOG_VERSION_STRING); -} - -/* Similar to virGetHostname() but avoids use of error - * reporting APIs or logging APIs, to prevent recursion - */ -static void -virLogHostnameString(char **rawmsg, - char **msg) -{ - char *hoststr; - - hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); - - virLogFormatString(msg, 0, NULL, VIR_LOG_INFO, hoststr); - *rawmsg = hoststr; -} - - static void virLogSourceUpdate(virLogSource *source) { @@ -479,6 +455,33 @@ virLogSourceUpdate(virLogSource *source) } +/** + * virLogOneInitMsg: + * + * @str: the "raw" form of the string that's going to be logged + * + * (the args are all described in the caller - virLogToOneTarget() + * @timestamp,@outputFunc, @data + * + * send one "init message" (the lines that are at the beginning of the + * log when a new daemon starts) to one target. This just creates the + * "fancy" version of the string with thread-id and priority, and + * sends that, along with the "raw" version of the string, to the log + * target at INFO level. + */ +static void virLogOneInitMsg(const char *timestamp, + const char *str, + virLogOutputFunc outputFunc, + void *data) +{ + g_autofree char *msg = NULL; + + virLogFormatString(&msg, 0, NULL, VIR_LOG_INFO, str); + outputFunc(&virLogSelf, VIR_LOG_INFO, __FILE__, __LINE__, __func__, + timestamp, NULL, str, msg, data); +} + + /** * virLogToOneTarget: * @@ -522,24 +525,18 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { - const char *rawinitmsg; - char *hoststr = NULL; - char *initmsg = NULL; - - virLogVersionString(&rawinitmsg, &initmsg); - outputFunc(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, rawinitmsg, initmsg, - data); - VIR_FREE(initmsg); - - virLogHostnameString(&hoststr, &initmsg); - outputFunc(&virLogSelf, VIR_LOG_INFO, - __FILE__, __LINE__, __func__, - timestamp, NULL, hoststr, initmsg, - data); - VIR_FREE(hoststr); - VIR_FREE(initmsg); + g_autofree char *hoststr = NULL; + + /* put some useful info at the top of the log. Avoid calling + * any function that might end up reporting an error or + * otherwise logging something, to prevent recursion. + */ + + virLogOneInitMsg(timestamp, VIR_LOG_VERSION_STRING, outputFunc, data); + + hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); + virLogOneInitMsg(timestamp, hoststr, outputFunc, data); + needInit = false; } -- 2.53.0
On Mon, Apr 06, 2026 at 23:29:18 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
The same thing happens for each line of the log banner:
1) A helper function is called that a) creates a "raw" string (just the desired info, e.g. version string) and b) calls virLogFormatString() for create a "cooked" version of the string (containing thread-id and log priority) 2) the outputFunc for the target is called with strings (a) and (b)
By making a helper that does (1b) & (2), we can further reduce the amount of redundant code that needs to be written to add another line to the banner - now all we need to do is:
1) create the raw string 2) call the helper, sending it the raw string
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 81 +++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 42 deletions(-)
Ah, this addresses comment from 3/6.
diff --git a/src/util/virlog.c b/src/util/virlog.c index 7a9b1e75d5..4e95af047b 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -433,30 +433,6 @@ virLogFormatString(char **msg, }
-static void -virLogVersionString(const char **rawmsg, - char **msg) -{ - *rawmsg = VIR_LOG_VERSION_STRING; - virLogFormatString(msg, 0, NULL, VIR_LOG_INFO, VIR_LOG_VERSION_STRING); -} - -/* Similar to virGetHostname() but avoids use of error - * reporting APIs or logging APIs, to prevent recursion - */ -static void -virLogHostnameString(char **rawmsg, - char **msg) -{ - char *hoststr; - - hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); - - virLogFormatString(msg, 0, NULL, VIR_LOG_INFO, hoststr); - *rawmsg = hoststr; -} - - static void virLogSourceUpdate(virLogSource *source) { @@ -479,6 +455,33 @@ virLogSourceUpdate(virLogSource *source) }
+/** + * virLogOneInitMsg: + * + * @str: the "raw" form of the string that's going to be logged + * + * (the args are all described in the caller - virLogToOneTarget() + * @timestamp,@outputFunc, @data + * + * send one "init message" (the lines that are at the beginning of the + * log when a new daemon starts) to one target. This just creates the + * "fancy" version of the string with thread-id and priority, and + * sends that, along with the "raw" version of the string, to the log + * target at INFO level. + */ +static void virLogOneInitMsg(const char *timestamp, + const char *str, + virLogOutputFunc outputFunc, + void *data)
Please use the contemporary formatting. 'static void' on separate line, fuction name starting at col 1.
+{ + g_autofree char *msg = NULL; + + virLogFormatString(&msg, 0, NULL, VIR_LOG_INFO, str); + outputFunc(&virLogSelf, VIR_LOG_INFO, __FILE__, __LINE__, __func__, + timestamp, NULL, str, msg, data); +} + + /** * virLogToOneTarget: *
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Laine Stump <laine@redhat.com> As libvirt is used more and more in unprivileged/session mode, file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as. The idea is to expend this to include more generally useful info about the environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc)) Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL; /* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data); + if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + } needInit = false; } -- 2.53.0
On Mon, Apr 06, 2026 at 11:29:19PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
As libvirt is used more and more in unprivileged/session mode, file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as.
The idea is to expend this to include more generally useful info about the environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc))
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL;
/* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data);
+ if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + }
The logs are used outside daemon context, so these messages are somewhat too specific. I'd suggest just logging the user/group alone without the leading 'running in ... mode' With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
On 4/7/26 4:37 AM, Daniel P. Berrangé via Devel wrote:
On Mon, Apr 06, 2026 at 11:29:19PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
As libvirt is used more and more in unprivileged/session mode, file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as.
The idea is to expend this to include more generally useful info about the environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc))
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL;
/* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data);
+ if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + }
The logs are used outside daemon context, so these messages are somewhat too specific. I'd suggest just logging the user/group alone without the leading 'running in ... mode'
Makes sense. I've just moved the two things up onto the same line with the hostname. What about the environment variables in patch 6? Too much? Not enough?
On Tue, Apr 07, 2026 at 03:02:11PM -0400, Laine Stump wrote:
On 4/7/26 4:37 AM, Daniel P. Berrangé via Devel wrote:
On Mon, Apr 06, 2026 at 11:29:19PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
As libvirt is used more and more in unprivileged/session mode, file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as.
The idea is to expend this to include more generally useful info about the environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc))
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL; /* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data); + if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + }
The logs are used outside daemon context, so these messages are somewhat too specific. I'd suggest just logging the user/group alone without the leading 'running in ... mode'
Makes sense. I've just moved the two things up onto the same line with the hostname. What about the environment variables in patch 6? Too much? Not enough?
Hard to say, but I'm pretty sure we've seen bugs with libguestfs where knowing the XDG_* env vars would have made diagnosis quicker, so that probably points to including them. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
On Tue, Apr 07, 2026 at 08:30:08PM +0100, Daniel P. Berrangé via Devel wrote:
Hard to say, but I'm pretty sure we've seen bugs with libguestfs where knowing the XDG_* env vars would have made diagnosis quicker, so that probably points to including them.
Several times, with the most recent ones being RHEL-70222 and RHEL-105490 (both already mentioned in this thread). Also there is a some similar problem with passt, resolved through documentation: https://passt.top/passt/commit/?id=c54ef9e8358db1a720c849612a32e52340d7a548 In libguestfs-test-tool we log XDG_RUNTIME_DIR: https://github.com/libguestfs/libguestfs/blob/a8ff3487bbf6c37b83e1d6eec32659... For background reading, this bug (in systemd): https://bugzilla.redhat.com/show_bug.cgi?id=967509 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Mon, Apr 06, 2026 at 11:29:19PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
As libvirt is used more and more in unprivileged/session mode,
"more and more"? We've been doing it like that for decades!
file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as.
The idea is to expend this to include more generally useful info about the
s/expend/expand/
environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc))
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL;
/* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data);
+ if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + } needInit = false; }
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 4/8/26 8:45 AM, Richard W.M. Jones via Devel wrote:
On Mon, Apr 06, 2026 at 11:29:19PM -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
As libvirt is used more and more in unprivileged/session mode,
"more and more"? We've been doing it like that for decades!
Yes, but now other people are catching on too :-) You were a true pioneer!! (P.S. Stop using that word "decades". Makes me feel old, and I already have enough reasons to feel old :-P) (says the guy who calls you a "pioneer"...)
file/socket permission errors have become more common. This patch adds an initial line to the log banner (the first thing sent to every log target after the process starts) stating whether the process is running privileged (as root) or unprivileged/session mode, and if the latter, it also provides the username and uid the process is running as.
The idea is to expend this to include more generally useful info about the
s/expend/expand/
environment we're running in. (We just need to remember that in this context we can't call anything that could lead to recursively calling the logging system (i.e. you can't call any code that reports an error, or a VIR_WARN, etc))
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virlog.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4e95af047b..d5bd216241 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -525,6 +525,7 @@ virLogToOneTarget(virLogSource *source, bool *needInit) { if (needInit) { + uid_t uid = geteuid(); g_autofree char *hoststr = NULL;
/* put some useful info at the top of the log. Avoid calling @@ -537,6 +538,16 @@ virLogToOneTarget(virLogSource *source, hoststr = g_strdup_printf("hostname: %s", g_get_host_name()); virLogOneInitMsg(timestamp, hoststr, outputFunc, data);
+ if (uid == 0) { + virLogOneInitMsg(timestamp, "running in privileged/system mode", outputFunc, data); + } else { + g_autofree char *username = virGetUserName(uid); + g_autofree char *privstr = NULL; + + privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", + username, uid); + virLogOneInitMsg(timestamp, privstr, outputFunc, data); + } needInit = false; }
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Rich.
From: Laine Stump <laine@redhat.com> When running in session/unprivileged mode, nearly all paths are prefixed with the returns from one of the g_get_user_*_dir() functions, which in turn base their selected paths on the settings of a few items in the user's environment ($XDG_*, or a subdirectory of $HOME if the $XDG_* isn't set). This patch logs the settings of these environment variables and directories in the log banner in an attempt to help diagnose the problem when a file/socket open/create fails. Documentation for the glib g_get_user_*_dir() functions used by libvirt, and how they use the XDG Base Directory settings (along with $HOME) can be found here (current version as of the time of this patch): https://www.manpagez.com/html/glib/glib-2.56.0/glib-Miscellaneous-Utility-Fu... The XDG Base Directory Specification can be found here: https://specifications.freedesktop.org/basedir/latest/ Resolves: https://redhat.atlassian.net/browse/RHEL-70222 Resolves: https://redhat.atlassian.net/browse/RHEL-105490 Signed-off-by: Laine Stump <laine@redhat.com> --- We could obviously add more information here (or less); it's difficult to know where to draw the line. Also, the astute reviewer will notice that all this code is executed once for each log target - we could do it all once at a higher level and cache it if we really wanted to. I'm not sure if it's worth the trouble though). src/util/virlog.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index d5bd216241..472ef3b261 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -543,10 +543,44 @@ virLogToOneTarget(virLogSource *source, } else { g_autofree char *username = virGetUserName(uid); g_autofree char *privstr = NULL; + g_autofree char *envHOME = NULL; + g_autofree char *envXDG_RUNTIME_DIR = NULL; + g_autofree char *envXDG_CONFIG_HOME = NULL; + g_autofree char *envXDG_CACHE_HOME = NULL; + + g_autofree char *envstr1 = NULL; + g_autofree char *envstr2 = NULL; + g_autofree char *envstr3 = NULL; + g_autofree char *envstr4 = NULL; + + if (!(envHOME = g_strdup(g_getenv("HOME")))) + envHOME = g_strdup("(unset)"); + if (!(envXDG_RUNTIME_DIR = g_strdup(g_getenv("XDG_RUNTIME_DIR")))) + envXDG_RUNTIME_DIR = g_strdup("(unset)"); + if (!(envXDG_CONFIG_HOME = g_strdup(g_getenv("XDG_CONFIG_HOME")))) + envXDG_CONFIG_HOME = g_strdup("(unset)"); + if (!(envXDG_CACHE_HOME = g_strdup(g_getenv("XDG_CACHE_HOME")))) + envXDG_CACHE_HOME = g_strdup("(unset)"); privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", username, uid); virLogOneInitMsg(timestamp, privstr, outputFunc, data); + + envstr1 = g_strdup_printf("environment: HOME='%s' (g_get_home_dir: '%s'", + envHOME, g_get_home_dir()); + virLogOneInitMsg(timestamp, envstr1, outputFunc, data); + + envstr2 = g_strdup_printf(" XDG_RUNTIME_DIR='%s' (g_get_user_runtime_dir: '%s')", + envXDG_RUNTIME_DIR, g_get_user_runtime_dir()); + virLogOneInitMsg(timestamp, envstr2, outputFunc, data); + + envstr3 = g_strdup_printf(" XDG_CONFIG_HOME='%s' (g_get_user_config_dir: '%s')", + envXDG_CONFIG_HOME, g_get_user_config_dir()); + virLogOneInitMsg(timestamp, envstr3, outputFunc, data); + + envstr4 = g_strdup_printf(" XDG_CACHE_HOME='%s' (g_get_user_cache_dir: '%s')", + envXDG_CACHE_HOME, g_get_user_cache_dir()); + virLogOneInitMsg(timestamp, envstr4, outputFunc, data); } needInit = false; } -- 2.53.0
On Mon, Apr 06, 2026 at 23:29:20 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
When running in session/unprivileged mode, nearly all paths are prefixed with the returns from one of the g_get_user_*_dir() functions, which in turn base their selected paths on the settings of a few items in the user's environment ($XDG_*, or a subdirectory of $HOME if the $XDG_* isn't set).
This patch logs the settings of these environment variables and directories in the log banner in an attempt to help diagnose the problem when a file/socket open/create fails.
Documentation for the glib g_get_user_*_dir() functions used by libvirt, and how they use the XDG Base Directory settings (along with $HOME) can be found here (current version as of the time of this patch):
https://www.manpagez.com/html/glib/glib-2.56.0/glib-Miscellaneous-Utility-Fu...
The XDG Base Directory Specification can be found here:
https://specifications.freedesktop.org/basedir/latest/
Resolves: https://redhat.atlassian.net/browse/RHEL-70222 Resolves: https://redhat.atlassian.net/browse/RHEL-105490 Signed-off-by: Laine Stump <laine@redhat.com> ---
We could obviously add more information here (or less); it's difficult to know where to draw the line. Also, the astute reviewer will notice that all this code is executed once for each log target - we could do it all once at a higher level and cache it if we really wanted to. I'm not sure if it's worth the trouble though).
src/util/virlog.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index d5bd216241..472ef3b261 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -543,10 +543,44 @@ virLogToOneTarget(virLogSource *source, } else { g_autofree char *username = virGetUserName(uid); g_autofree char *privstr = NULL; + g_autofree char *envHOME = NULL; + g_autofree char *envXDG_RUNTIME_DIR = NULL; + g_autofree char *envXDG_CONFIG_HOME = NULL; + g_autofree char *envXDG_CACHE_HOME = NULL; + + g_autofree char *envstr1 = NULL; + g_autofree char *envstr2 = NULL; + g_autofree char *envstr3 = NULL; + g_autofree char *envstr4 = NULL; + + if (!(envHOME = g_strdup(g_getenv("HOME")))) + envHOME = g_strdup("(unset)"); + if (!(envXDG_RUNTIME_DIR = g_strdup(g_getenv("XDG_RUNTIME_DIR")))) + envXDG_RUNTIME_DIR = g_strdup("(unset)"); + if (!(envXDG_CONFIG_HOME = g_strdup(g_getenv("XDG_CONFIG_HOME")))) + envXDG_CONFIG_HOME = g_strdup("(unset)"); + if (!(envXDG_CACHE_HOME = g_strdup(g_getenv("XDG_CACHE_HOME")))) + envXDG_CACHE_HOME = g_strdup("(unset)");
privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", username, uid); virLogOneInitMsg(timestamp, privstr, outputFunc, data); + + envstr1 = g_strdup_printf("environment: HOME='%s' (g_get_home_dir: '%s'", + envHOME, g_get_home_dir()); + virLogOneInitMsg(timestamp, envstr1, outputFunc, data); + + envstr2 = g_strdup_printf(" XDG_RUNTIME_DIR='%s' (g_get_user_runtime_dir: '%s')", + envXDG_RUNTIME_DIR, g_get_user_runtime_dir()); + virLogOneInitMsg(timestamp, envstr2, outputFunc, data); + + envstr3 = g_strdup_printf(" XDG_CONFIG_HOME='%s' (g_get_user_config_dir: '%s')", + envXDG_CONFIG_HOME, g_get_user_config_dir()); + virLogOneInitMsg(timestamp, envstr3, outputFunc, data); + + envstr4 = g_strdup_printf(" XDG_CACHE_HOME='%s' (g_get_user_cache_dir: '%s')", + envXDG_CACHE_HOME, g_get_user_cache_dir()); + virLogOneInitMsg(timestamp, envstr4, outputFunc, data);
So the result is: 2026-04-07 20:50:24.676+0000: 2303079: info : environment: HOME='/home/pipo' (g_get_home_dir: '/home/pipo' 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_RUNTIME_DIR='/run/user/1000' (g_get_user_runtime_dir: '/run/user/1000') 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_CONFIG_HOME='(unset)' (g_get_user_config_dir: '/home/pipo/.config') 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_CACHE_HOME='(unset)' (g_get_user_cache_dir: '/home/pipo/.cache') I'm not a fan of the attempt to align it. The first line is missing a closing parenthesis. I'm also not a fan of the glib function names being used as identifiers. I think I'd prefer if: - you drop the 'environment' prefix and just dump the variables - avoid the block alignment - skip the glib names, instead use the actual value as the first entry with something more generic. e.g.: home dir: '/home/pipo/' (HOME='/home/pipo') runtime dir: '/run/user/1000' (XDG_RUNTIME_DIR='/run/user/1000') config dir: '/home/pipo/.config' (XDG_CONFIG_HOME='(unset)') etc and remove also the mention of the glib stuff from the commit message. Instead put an example of the log there.
On 4/7/26 5:01 PM, Peter Krempa via Devel wrote:
On Mon, Apr 06, 2026 at 23:29:20 -0400, Laine Stump via Devel wrote:
From: Laine Stump <laine@redhat.com>
When running in session/unprivileged mode, nearly all paths are prefixed with the returns from one of the g_get_user_*_dir() functions, which in turn base their selected paths on the settings of a few items in the user's environment ($XDG_*, or a subdirectory of $HOME if the $XDG_* isn't set).
This patch logs the settings of these environment variables and directories in the log banner in an attempt to help diagnose the problem when a file/socket open/create fails.
Documentation for the glib g_get_user_*_dir() functions used by libvirt, and how they use the XDG Base Directory settings (along with $HOME) can be found here (current version as of the time of this patch):
https://www.manpagez.com/html/glib/glib-2.56.0/glib-Miscellaneous-Utility-Fu...
The XDG Base Directory Specification can be found here:
https://specifications.freedesktop.org/basedir/latest/
Resolves: https://redhat.atlassian.net/browse/RHEL-70222 Resolves: https://redhat.atlassian.net/browse/RHEL-105490 Signed-off-by: Laine Stump <laine@redhat.com> ---
We could obviously add more information here (or less); it's difficult to know where to draw the line. Also, the astute reviewer will notice that all this code is executed once for each log target - we could do it all once at a higher level and cache it if we really wanted to. I'm not sure if it's worth the trouble though).
src/util/virlog.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c index d5bd216241..472ef3b261 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -543,10 +543,44 @@ virLogToOneTarget(virLogSource *source, } else { g_autofree char *username = virGetUserName(uid); g_autofree char *privstr = NULL; + g_autofree char *envHOME = NULL; + g_autofree char *envXDG_RUNTIME_DIR = NULL; + g_autofree char *envXDG_CONFIG_HOME = NULL; + g_autofree char *envXDG_CACHE_HOME = NULL; + + g_autofree char *envstr1 = NULL; + g_autofree char *envstr2 = NULL; + g_autofree char *envstr3 = NULL; + g_autofree char *envstr4 = NULL; + + if (!(envHOME = g_strdup(g_getenv("HOME")))) + envHOME = g_strdup("(unset)"); + if (!(envXDG_RUNTIME_DIR = g_strdup(g_getenv("XDG_RUNTIME_DIR")))) + envXDG_RUNTIME_DIR = g_strdup("(unset)"); + if (!(envXDG_CONFIG_HOME = g_strdup(g_getenv("XDG_CONFIG_HOME")))) + envXDG_CONFIG_HOME = g_strdup("(unset)"); + if (!(envXDG_CACHE_HOME = g_strdup(g_getenv("XDG_CACHE_HOME")))) + envXDG_CACHE_HOME = g_strdup("(unset)");
privstr = g_strdup_printf("running in unprivileged/session mode, user: %s, uid: %u", username, uid); virLogOneInitMsg(timestamp, privstr, outputFunc, data); + + envstr1 = g_strdup_printf("environment: HOME='%s' (g_get_home_dir: '%s'", + envHOME, g_get_home_dir()); + virLogOneInitMsg(timestamp, envstr1, outputFunc, data); + + envstr2 = g_strdup_printf(" XDG_RUNTIME_DIR='%s' (g_get_user_runtime_dir: '%s')", + envXDG_RUNTIME_DIR, g_get_user_runtime_dir()); + virLogOneInitMsg(timestamp, envstr2, outputFunc, data); + + envstr3 = g_strdup_printf(" XDG_CONFIG_HOME='%s' (g_get_user_config_dir: '%s')", + envXDG_CONFIG_HOME, g_get_user_config_dir()); + virLogOneInitMsg(timestamp, envstr3, outputFunc, data); + + envstr4 = g_strdup_printf(" XDG_CACHE_HOME='%s' (g_get_user_cache_dir: '%s')", + envXDG_CACHE_HOME, g_get_user_cache_dir()); + virLogOneInitMsg(timestamp, envstr4, outputFunc, data);
So the result is:
2026-04-07 20:50:24.676+0000: 2303079: info : environment: HOME='/home/pipo' (g_get_home_dir: '/home/pipo' 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_RUNTIME_DIR='/run/user/1000' (g_get_user_runtime_dir: '/run/user/1000') 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_CONFIG_HOME='(unset)' (g_get_user_config_dir: '/home/pipo/.config') 2026-04-07 20:50:24.676+0000: 2303079: info : XDG_CACHE_HOME='(unset)' (g_get_user_cache_dir: '/home/pipo/.cache')
I'm not a fan of the attempt to align it.
Yeah, seemed better in my head but does look kind of messed up especially if the lines get wrapped.
The first line is missing a closing parenthesis.
My attention to detail is really failing! :-O
I'm also not a fan of the glib function names being used as identifiers.
Heh. I actually originally used, e.g. "runtime dir", but thought it might be nice to make the connection to the glib API more evident. I'm no huge fan of it, but at some point I had to stop self-bikeshedding and post it (ie having other people give their opinions on how to do it differently is *exactly* what I was looking for, so thanks! (to you and to Daniel))
I think I'd prefer if: - you drop the 'environment' prefix and just dump the variables - avoid the block alignment - skip the glib names, instead use the actual value as the first entry with something more generic. e.g.:
home dir: '/home/pipo/' (HOME='/home/pipo') runtime dir: '/run/user/1000' (XDG_RUNTIME_DIR='/run/user/1000') config dir: '/home/pipo/.config' (XDG_CONFIG_HOME='(unset)')
etc and remove also the mention of the glib stuff from the commit message. Instead put an example of the log there.
(I had intended to put an example in the commit log, not sure why I didn't) That looks much better! One thing though - we call g_get_user_cache_dir() (which uses XDG_CACHE_HOME), but then end up using it almost exclusively for logfiles, so as long as I'm not using the exact API name, I may as well label it more functionally, as "log dir" (better suits our usage) instead of "cache dir" (matching the glib API)
participants (4)
-
Daniel P. Berrangé -
Laine Stump -
Peter Krempa -
Richard W.M. Jones