[libvirt] [PATCH v2 0/4] Rewrite virGetUser*Directory() functions using g_get_*_dir()
By rewriting virGetUser*Directory() functions using g_get_*_dir() functions allows us to drop all the different implementations we keep, as GLib already takes care of those for us. Changes since v1: https://www.redhat.com/archives/libvir-list/2019-December/msg01055.html - Don't check for the return of g_get_*_dir(), as it cannot be NULL; Fabiano Fidêncio (4): util: Rewrite virGetUserDirectory() using g_get_home_dir() util: Rewrite virGetUserConfigDirectory() using g_get_user_config_dir() util: Rewrite virGetUserCacheDirectory() using g_get_user_cache_dir() util: Rewrite virGetUserRuntimeDirectory() using g_get_user_runtime_dir() src/util/virutil.c | 125 +++++++-------------------------------------- 1 file changed, 19 insertions(+), 106 deletions(-) -- 2.23.0
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir()); } -- 2.23.0
On 12/17/19 11:41 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir()); }
This is supposed to be returning $HOME, not $HOME/libvirt. IMO leave this one as it is, since it's tied into the larger getent handling - Cole
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g. g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
}
-- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Kind regards / Beste Grüße Marc Hartmayer
IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Indeed. I'll do the change before pushing, in case Cole ACKs the v3. :-) Best Regards, -- Fabiano Fidêncio
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Good catch There's also g_build_filename which sounds simpler. Any reason for one over the other? - Cole
On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Good catch
There's also g_build_filename which sounds simpler. Any reason for one over the other?
Based on the docs g_build_filename looks better, as it keeps '/' vs '\' consistent on windows. Aside from that, their internal impl is basically the same. 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 :|
On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Good catch
There's also g_build_filename which sounds simpler. Any reason for one over the other?
Based on the docs g_build_filename looks better, as it keeps '/' vs '\' consistent on windows. Aside from that, their internal impl is basically the same.
Okay, I'll use g_build_filename() then. I wonder whether consistently using g_build_filename() wherever it's possible should be added to the BiteSizedTasks as well.
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 :|
On 12/18/19 11:12 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed1f696e37..8c255abd7f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -582,7 +582,7 @@ virGetHostnameQuiet(void) char * virGetUserDirectory(void) { - return virGetUserDirectoryByUID(geteuid()); + return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'. E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Good catch
There's also g_build_filename which sounds simpler. Any reason for one over the other?
Based on the docs g_build_filename looks better, as it keeps '/' vs '\' consistent on windows. Aside from that, their internal impl is basically the same.
Okay, I'll use g_build_filename() then. I wonder whether consistently using g_build_filename() wherever it's possible should be added to the BiteSizedTasks as well.
Certainly in places where we have windows specific code. I feel like we must get path separator stuff wrong already in code that is compiled for windows (netclient at least), but maybe there's some gnulib magic that is handling path conversion for us? - Cole
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 8c255abd7f..63680974b8 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -586,6 +586,12 @@ virGetUserDirectory(void) } +char *virGetUserConfigDirectory(void) +{ + return g_strdup_printf("%s/libvirt", g_get_user_config_dir()); +} + + #ifdef HAVE_GETPWUID_R /* Look up fields from the user database for the given user. On * error, set errno, report the error if not instructed otherwise via @quiet, @@ -754,11 +760,6 @@ static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) return ret; } -char *virGetUserConfigDirectory(void) -{ - return virGetXDGDirectory("XDG_CONFIG_HOME", ".config"); -} - char *virGetUserCacheDirectory(void) { return virGetXDGDirectory("XDG_CACHE_HOME", ".cache"); @@ -1187,21 +1188,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } -char * -virGetUserConfigDirectory(void) -{ - char *ret; - if (virGetWin32SpecialFolder(CSIDL_LOCAL_APPDATA, &ret) < 0) - return NULL; - - if (!ret) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine config directory")); - return NULL; - } - return ret; -} - char * virGetUserCacheDirectory(void) { @@ -1242,15 +1228,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } -char * -virGetUserConfigDirectory(void) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virGetUserConfigDirectory is not available")); - - return NULL; -} - char * virGetUserCacheDirectory(void) { -- 2.23.0
On 12/17/19 11:41 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index 8c255abd7f..63680974b8 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -586,6 +586,12 @@ virGetUserDirectory(void) }
+char *virGetUserConfigDirectory(void) +{ + return g_strdup_printf("%s/libvirt", g_get_user_config_dir()); +} + + #ifdef HAVE_GETPWUID_R /* Look up fields from the user database for the given user. On * error, set errno, report the error if not instructed otherwise via @quiet, @@ -754,11 +760,6 @@ static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) return ret; }
-char *virGetUserConfigDirectory(void) -{ - return virGetXDGDirectory("XDG_CONFIG_HOME", ".config"); -} - char *virGetUserCacheDirectory(void) { return virGetXDGDirectory("XDG_CACHE_HOME", ".cache"); @@ -1187,21 +1188,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; }
-char * -virGetUserConfigDirectory(void) -{ - char *ret; - if (virGetWin32SpecialFolder(CSIDL_LOCAL_APPDATA, &ret) < 0) - return NULL; - - if (!ret) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine config directory")); - return NULL; - } - return ret; -} -
Unfortunately looks like the windows implementations do _not_ add '/libvirt' to the returned directory. I'm doubtful anyone is depending on these paths being stable, but for safety we should differentiate this. I think it's fine to do: #ifdef WIN32 return g_strdup(g_get_blah()); #else return g_strdup_print(...); #endif Rather than have two different functions Also, a couple ideas for a follow up series: all the error handling from the GetUser*Directory functions can be removed after this. Also, the it looks like the windows impl of virGetUserDirectoryByUID can be entirely replaced with g_get_user_home(), which allows dropping some more windows helpers (the code was apparently adapted from glib anyways) - Cole
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 53 ++++++---------------------------------------- 1 file changed, 6 insertions(+), 47 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 63680974b8..9485316e05 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -592,6 +592,12 @@ char *virGetUserConfigDirectory(void) } +char *virGetUserCacheDirectory(void) +{ + return g_strdup_printf("%s/libvirt", g_get_user_cache_dir()); +} + + #ifdef HAVE_GETPWUID_R /* Look up fields from the user database for the given user. On * error, set errno, report the error if not instructed otherwise via @quiet, @@ -742,29 +748,6 @@ char *virGetUserShell(uid_t uid) } -static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) -{ - const char *path = getenv(xdgenvname); - char *ret = NULL; - char *home = NULL; - - if (path && path[0]) { - ret = g_strdup_printf("%s/libvirt", path); - } else { - home = virGetUserDirectory(); - if (home) - ret = g_strdup_printf("%s/%s/libvirt", home, xdgdefdir); - } - - VIR_FREE(home); - return ret; -} - -char *virGetUserCacheDirectory(void) -{ - return virGetXDGDirectory("XDG_CACHE_HOME", ".cache"); -} - char *virGetUserRuntimeDirectory(void) { const char *path = getenv("XDG_RUNTIME_DIR"); @@ -1188,21 +1171,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } -char * -virGetUserCacheDirectory(void) -{ - char *ret; - if (virGetWin32SpecialFolder(CSIDL_INTERNET_CACHE, &ret) < 0) - return NULL; - - if (!ret) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine config directory")); - return NULL; - } - return ret; -} - char * virGetUserRuntimeDirectory(void) { @@ -1228,15 +1196,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } -char * -virGetUserCacheDirectory(void) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virGetUserCacheDirectory is not available")); - - return NULL; -} - char * virGetUserRuntimeDirectory(void) { -- 2.23.0
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virutil.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 9485316e05..10bf96f193 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -598,6 +598,12 @@ char *virGetUserCacheDirectory(void) } +char *virGetUserRuntimeDirectory(void) +{ + return g_strdup_printf("%s/libvirt", g_get_user_runtime_dir()); +} + + #ifdef HAVE_GETPWUID_R /* Look up fields from the user database for the given user. On * error, set errno, report the error if not instructed otherwise via @quiet, @@ -748,20 +754,6 @@ char *virGetUserShell(uid_t uid) } -char *virGetUserRuntimeDirectory(void) -{ - const char *path = getenv("XDG_RUNTIME_DIR"); - - if (!path || !path[0]) { - return virGetUserCacheDirectory(); - } else { - char *ret; - - ret = g_strdup_printf("%s/libvirt", path); - return ret; - } -} - char *virGetUserName(uid_t uid) { char *ret; @@ -1171,12 +1163,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } -char * -virGetUserRuntimeDirectory(void) -{ - return virGetUserCacheDirectory(); -} - # else /* !HAVE_GETPWUID_R && !WIN32 */ char * virGetUserDirectoryByUID(uid_t uid G_GNUC_UNUSED) @@ -1195,15 +1181,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED) return NULL; } - -char * -virGetUserRuntimeDirectory(void) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virGetUserRuntimeDirectory is not available")); - - return NULL; -} # endif /* ! HAVE_GETPWUID_R && ! WIN32 */ char * -- 2.23.0
participants (4)
-
Cole Robinson -
Daniel P. Berrangé -
Fabiano Fidêncio -
Marc Hartmayer