1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 00/42] Cleanups after adopting g_get_*_dir()
  4. View patch

[libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()

Fabiano Fidêncio posted 42 patches 6 years, 1 month ago
[libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Fabiano Fidêncio 6 years, 1 month ago 
virGetUserDirectory() *never* *ever* returns NULL, making the checks for
it completely unnecessary.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
---
 src/rpc/virnetclient.c     | 12 ++++--------
 src/rpc/virnettlscontext.c | 12 ------------
 2 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 1b882a261a..40e5fa35e2 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -466,10 +466,8 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
         privkey = g_strdup(privkeyPath);
     } else {
         homedir = virGetUserDirectory();
-        if (homedir) {
-            if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
-                goto no_memory;
-        }
+        if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
+            goto no_memory;
     }
 
     if (!authMethods) {
@@ -566,10 +564,8 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
         privkey = g_strdup(privkeyPath);
     } else {
         homedir = virGetUserDirectory();
-        if (homedir) {
-            if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
-                goto no_memory;
-        }
+        if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
+            goto no_memory;
     }
 
     if (!authMethods) {
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index ec9dd35c46..08944f6771 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
          */
         userdir = virGetUserDirectory();
 
-        if (!userdir)
-            goto error;
-
         user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
 
         VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
@@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
     VIR_FREE(userdir);
 
     return 0;
-
- error:
-    VIR_FREE(*cacert);
-    VIR_FREE(*cacrl);
-    VIR_FREE(*key);
-    VIR_FREE(*cert);
-    VIR_FREE(user_pki_path);
-    VIR_FREE(userdir);
-    return -1;
 }
 
 
-- 
2.24.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Ján Tomko 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
>virGetUserDirectory() *never* *ever* returns NULL, making the checks for
>it completely unnecessary.
>
>Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
>---
> src/rpc/virnetclient.c     | 12 ++++--------
> src/rpc/virnettlscontext.c | 12 ------------
> 2 files changed, 4 insertions(+), 20 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Pavel Hrdina 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
> virGetUserDirectory() *never* *ever* returns NULL, making the checks for
> it completely unnecessary.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> ---
>  src/rpc/virnetclient.c     | 12 ++++--------
>  src/rpc/virnettlscontext.c | 12 ------------
>  2 files changed, 4 insertions(+), 20 deletions(-)

[...]

> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index ec9dd35c46..08944f6771 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
>           */
>          userdir = virGetUserDirectory();
>  
> -        if (!userdir)
> -            goto error;
> -
>          user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
>  
>          VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
> @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
>      VIR_FREE(userdir);
>  
>      return 0;
> -
> - error:
> -    VIR_FREE(*cacert);
> -    VIR_FREE(*cacrl);
> -    VIR_FREE(*key);
> -    VIR_FREE(*cert);
> -    VIR_FREE(user_pki_path);
> -    VIR_FREE(userdir);
> -    return -1;

This doesn't look right.  Looks like some leftover from rebase where
you wanted to use g_autofree.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Fabiano Fidêncio 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
>
> On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
> > virGetUserDirectory() *never* *ever* returns NULL, making the checks for
> > it completely unnecessary.
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> > ---
> >  src/rpc/virnetclient.c     | 12 ++++--------
> >  src/rpc/virnettlscontext.c | 12 ------------
> >  2 files changed, 4 insertions(+), 20 deletions(-)
>
> [...]
>
> > diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> > index ec9dd35c46..08944f6771 100644
> > --- a/src/rpc/virnettlscontext.c
> > +++ b/src/rpc/virnettlscontext.c
> > @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> >           */
> >          userdir = virGetUserDirectory();
> >
> > -        if (!userdir)
> > -            goto error;
> > -
> >          user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
> >
> >          VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
> > @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> >      VIR_FREE(userdir);
> >
> >      return 0;
> > -
> > - error:
> > -    VIR_FREE(*cacert);
> > -    VIR_FREE(*cacrl);
> > -    VIR_FREE(*key);
> > -    VIR_FREE(*cert);
> > -    VIR_FREE(user_pki_path);
> > -    VIR_FREE(userdir);
> > -    return -1;
>
> This doesn't look right.  Looks like some leftover from rebase where
> you wanted to use g_autofree.

No, actually not. The only usage of `goto error` was when checking the
result of virGetUserDirectory().
By removing the check, we have also to remove the label.

Best Regards,
-- 
Fabiano Fidêncio


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Pavel Hrdina 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
> On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> > On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
> > > virGetUserDirectory() *never* *ever* returns NULL, making the checks for
> > > it completely unnecessary.
> > >
> > > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> > > ---
> > >  src/rpc/virnetclient.c     | 12 ++++--------
> > >  src/rpc/virnettlscontext.c | 12 ------------
> > >  2 files changed, 4 insertions(+), 20 deletions(-)
> >
> > [...]
> >
> > > diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> > > index ec9dd35c46..08944f6771 100644
> > > --- a/src/rpc/virnettlscontext.c
> > > +++ b/src/rpc/virnettlscontext.c
> > > @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> > >           */
> > >          userdir = virGetUserDirectory();
> > >
> > > -        if (!userdir)
> > > -            goto error;
> > > -
> > >          user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
> > >
> > >          VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
> > > @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> > >      VIR_FREE(userdir);
> > >
> > >      return 0;
> > > -
> > > - error:
> > > -    VIR_FREE(*cacert);
> > > -    VIR_FREE(*cacrl);
> > > -    VIR_FREE(*key);
> > > -    VIR_FREE(*cert);
> > > -    VIR_FREE(user_pki_path);
> > > -    VIR_FREE(userdir);
> > > -    return -1;
> >
> > This doesn't look right.  Looks like some leftover from rebase where
> > you wanted to use g_autofree.
> 
> No, actually not. The only usage of `goto error` was when checking the
> result of virGetUserDirectory().
> By removing the check, we have also to remove the label.

Label yes, the VIR_FREE no as it would leak the memory.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Ján Tomko 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 05:25:59PM +0100, Pavel Hrdina wrote:
>On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
>> On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
>> >
>> > On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
>> > > virGetUserDirectory() *never* *ever* returns NULL, making the checks for
>> > > it completely unnecessary.
>> > >
>> > > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
>> > > ---
>> > >  src/rpc/virnetclient.c     | 12 ++++--------
>> > >  src/rpc/virnettlscontext.c | 12 ------------
>> > >  2 files changed, 4 insertions(+), 20 deletions(-)
>> >
>> > [...]
>> >
>> > > diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
>> > > index ec9dd35c46..08944f6771 100644
>> > > --- a/src/rpc/virnettlscontext.c
>> > > +++ b/src/rpc/virnettlscontext.c
>> > > @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
>> > >           */
>> > >          userdir = virGetUserDirectory();
>> > >
>> > > -        if (!userdir)
>> > > -            goto error;
>> > > -
>> > >          user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
>> > >
>> > >          VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
>> > > @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
>> > >      VIR_FREE(userdir);
>> > >
>> > >      return 0;
>> > > -
>> > > - error:
>> > > -    VIR_FREE(*cacert);
>> > > -    VIR_FREE(*cacrl);
>> > > -    VIR_FREE(*key);
>> > > -    VIR_FREE(*cert);
>> > > -    VIR_FREE(user_pki_path);
>> > > -    VIR_FREE(userdir);
>> > > -    return -1;
>> >
>> > This doesn't look right.  Looks like some leftover from rebase where
>> > you wanted to use g_autofree.
>>
>> No, actually not. The only usage of `goto error` was when checking the
>> result of virGetUserDirectory().
>> By removing the check, we have also to remove the label.
>
>Label yes, the VIR_FREE no as it would leak the memory.
>

There's "return 0;" right before that label.

Jano

>Pavel



>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/42] rpc: Don't check the output of virGetUserDirectory()
Posted by Pavel Hrdina 6 years, 1 month ago 
On Thu, Dec 19, 2019 at 05:42:14PM +0100, Ján Tomko wrote:
> On Thu, Dec 19, 2019 at 05:25:59PM +0100, Pavel Hrdina wrote:
> > On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
> > > On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
> > > >
> > > > On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
> > > > > virGetUserDirectory() *never* *ever* returns NULL, making the checks for
> > > > > it completely unnecessary.
> > > > >
> > > > > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> > > > > ---
> > > > >  src/rpc/virnetclient.c     | 12 ++++--------
> > > > >  src/rpc/virnettlscontext.c | 12 ------------
> > > > >  2 files changed, 4 insertions(+), 20 deletions(-)
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> > > > > index ec9dd35c46..08944f6771 100644
> > > > > --- a/src/rpc/virnettlscontext.c
> > > > > +++ b/src/rpc/virnettlscontext.c
> > > > > @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> > > > >           */
> > > > >          userdir = virGetUserDirectory();
> > > > >
> > > > > -        if (!userdir)
> > > > > -            goto error;
> > > > > -
> > > > >          user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
> > > > >
> > > > >          VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path);
> > > > > @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
> > > > >      VIR_FREE(userdir);
> > > > >
> > > > >      return 0;
> > > > > -
> > > > > - error:
> > > > > -    VIR_FREE(*cacert);
> > > > > -    VIR_FREE(*cacrl);
> > > > > -    VIR_FREE(*key);
> > > > > -    VIR_FREE(*cert);
> > > > > -    VIR_FREE(user_pki_path);
> > > > > -    VIR_FREE(userdir);
> > > > > -    return -1;
> > > >
> > > > This doesn't look right.  Looks like some leftover from rebase where
> > > > you wanted to use g_autofree.
> > > 
> > > No, actually not. The only usage of `goto error` was when checking the
> > > result of virGetUserDirectory().
> > > By removing the check, we have also to remove the label.
> > 
> > Label yes, the VIR_FREE no as it would leak the memory.
> > 
> 
> There's "return 0;" right before that label.

Oh, I see, two strings are freed and the remaining ones are
transferred to caller.  Somehow I missed that.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.