10:41 a.m.
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
10:41 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
1:49 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On 12/17/19 11:41 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
4:18 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio(a)redhat.com>
wrote:
Signed-off-by: Fabiano Fidêncio <fidencio(a)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(a)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
4:34 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer <mhartmay(a)linux.ibm.com> wrote:
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio(a)redhat.com>
wrote:
> Signed-off-by: Fabiano Fidêncio <fidencio(a)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
9:53 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
>
> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio <fidencio(a)redhat.com>
wrote:
>> Signed-off-by: Fabiano Fidêncio <fidencio(a)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
9:59 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
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(a)linux.ibm.com>
wrote:
>>
>> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
<fidencio(a)redhat.com> wrote:
>>> Signed-off-by: Fabiano Fidêncio <fidencio(a)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 :|
10:12 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé <berrange(a)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(a)linux.ibm.com>
wrote:
> >>
> >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
<fidencio(a)redhat.com> wrote:
> >>> Signed-off-by: Fabiano Fidêncio <fidencio(a)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 :|
10:27 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On 12/18/19 11:12 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé
<berrange(a)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(a)linux.ibm.com> wrote:
>>>>
>>>> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
<fidencio(a)redhat.com> wrote:
>>>>> Signed-off-by: Fabiano Fidêncio <fidencio(a)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
10:41 a.m.
New subject: [libvirt] [PATCH v2 2/4] util: Rewrite virGetUserConfigDirectory() using g_get_user_config_dir()
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
2:04 p.m.
New subject: [libvirt] [PATCH v2 2/4] util: Rewrite virGetUserConfigDirectory() using g_get_user_config_dir()
On 12/17/19 11:41 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
10:41 a.m.
New subject: [libvirt] [PATCH v2 3/4] util: Rewrite virGetUserCacheDirectory() using g_get_user_cache_dir()
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
10:41 a.m.
New subject: [libvirt] [PATCH v2 4/4] util: Rewrite virGetUserRuntimeDirectory() using g_get_user_runtime_dir()
Signed-off-by: Fabiano Fidêncio <fidencio(a)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
1801
days inactive
1802
days old
12 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrangé -
Fabiano Fidêncio -
Marc Hartmayer