On Mon, Apr 06, 2026 at 23:29:20 -0400, Laine Stump via Devel wrote:
> From: Laine Stump <[email protected]>
> 
> 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-Functions.php
> 
> 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 <[email protected]>
> ---
> 
> 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.

Reply via email to