Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()

2019-12-18 Thread Cole Robinson 
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()

2019-12-18 Thread Fabiano Fidêncio 
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()

2019-12-18 Thread Daniel P . Berrangé 
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()

2019-12-18 Thread Cole Robinson 
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()

2019-12-18 Thread Fabiano Fidêncio 
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()

2019-12-18 Thread Marc Hartmayer 
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()

2019-12-17 Thread Cole Robinson 
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()

2019-12-17 Thread Fabiano Fidêncio 
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