Re: [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é > 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 wrote: > > On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio > wrote: >> Signed-off-by: Fabiano Fidêncio >> --- >> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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é 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 > > > wrote: > > >> > > >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio > > >> wrote: > > >>> Signed-off-by: Fabiano Fidêncio > > >>> --- > > >>> 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 :| > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 > > wrote: > >> > >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio > >> wrote: > >>> Signed-off-by: Fabiano Fidêncio > >>> --- > >>> 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 > wrote: >> >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio >> wrote: >>> Signed-off-by: Fabiano Fidêncio >>> --- >>> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer wrote: > > On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio > wrote: > > Signed-off-by: Fabiano Fidêncio > > --- > > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 wrote: > Signed-off-by: Fabiano Fidêncio > --- > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 > --- > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
Signed-off-by: Fabiano Fidêncio --- 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list