Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
---
 src/rpc/virnetclient.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 47a17d30f7..75e653fec8 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -442,13 +442,13 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
     virNetClientPtr ret = NULL;
 
     virBuffer buf = VIR_BUFFER_INITIALIZER;
-    char *nc = NULL;
-    char *command = NULL;
+    g_autofree char *nc = NULL;
+    g_autofree char *command = NULL;
 
-    char *homedir = NULL;
-    char *confdir = NULL;
-    char *knownhosts = NULL;
-    char *privkey = NULL;
+    g_autofree char *homedir = NULL;
+    g_autofree char *confdir = NULL;
+    g_autofree char *knownhosts = NULL;
+    g_autofree char *privkey = NULL;
 
     /* Use default paths for known hosts an public keys if not provided */
     if (knownHostsPath) {
@@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
         goto cleanup;
 
  cleanup:
-    VIR_FREE(command);
-    VIR_FREE(privkey);
-    VIR_FREE(knownhosts);
-    VIR_FREE(homedir);
-    VIR_FREE(confdir);
-    VIR_FREE(nc);
     return ret;
 
  no_memory:
-- 
2.24.1

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

s/on/in/

On Thu, Dec 19, 2019 at 11:04:08AM +0100, Fabiano Fidêncio wrote:
>Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
>---
> src/rpc/virnetclient.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 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

On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> ---
>   src/rpc/virnetclient.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
[...]

>       if (knownHostsPath) {
> @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
>           goto cleanup;
>   
>    cleanup:
> -    VIR_FREE(command);
> -    VIR_FREE(privkey);
> -    VIR_FREE(knownhosts);
> -    VIR_FREE(homedir);
> -    VIR_FREE(confdir);
> -    VIR_FREE(nc);
>       return ret;
>   

This will leave a now unneeded 'cleanup' label:

  cleanup:
     return ret;


In a quick glance at the patch series I noticed that Patch 41 also leaves
an unused 'cleanup' label as well after the changes.


Thanks,


DHB

>    no_memory:
> 


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

On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
> > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> > ---
> >   src/rpc/virnetclient.c | 18 ++++++------------
> >   1 file changed, 6 insertions(+), 12 deletions(-)
> >
> [...]
>
> >       if (knownHostsPath) {
> > @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
> >           goto cleanup;
> >
> >    cleanup:
> > -    VIR_FREE(command);
> > -    VIR_FREE(privkey);
> > -    VIR_FREE(knownhosts);
> > -    VIR_FREE(homedir);
> > -    VIR_FREE(confdir);
> > -    VIR_FREE(nc);
> >       return ret;
> >
>
> This will leave a now unneeded 'cleanup' label:
>
>   cleanup:
>      return ret;
>
>
> In a quick glance at the patch series I noticed that Patch 41 also leaves
> an unused 'cleanup' label as well after the changes.

Daniel,

Please, take a look at the no_memory label. It calls the cleanup one.

Best Regards,
-- 
Fabiano Fidêncio


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

On 12/19/19 7:17 PM, Fabiano Fidêncio wrote:
> On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>>
>>
>> On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
>>> Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
>>> ---
>>>    src/rpc/virnetclient.c | 18 ++++++------------
>>>    1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>> [...]
>>
>>>        if (knownHostsPath) {
>>> @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
>>>            goto cleanup;
>>>
>>>     cleanup:
>>> -    VIR_FREE(command);
>>> -    VIR_FREE(privkey);
>>> -    VIR_FREE(knownhosts);
>>> -    VIR_FREE(homedir);
>>> -    VIR_FREE(confdir);
>>> -    VIR_FREE(nc);
>>>        return ret;
>>>
>>
>> This will leave a now unneeded 'cleanup' label:
>>
>>    cleanup:
>>       return ret;
>>
>>
>> In a quick glance at the patch series I noticed that Patch 41 also leaves
>> an unused 'cleanup' label as well after the changes.
> 
> Daniel,
> 
> Please, take a look at the no_memory label. It calls the cleanup one.

I am afraid I wasn't clear. By 'unneeded' I mean a label that is a simply 'return'
call. 'cleanup' is doing no cleanups anymore. So this this guy:

  no_memory:
     virReportOOMError();
     goto cleanup;

turns into:

  no_memory:
     virReportOOMError();
     return ret;


And in fact, since ret is initialized with NULL, and it will never be set before
a no_memory jump, you can even do:

  no_memory:
     virReportOOMError();
     return NULL;

All 'cleanup' jumps can be changed to 'return NULL' and the line that sets 'ret'
can turn into

    return virNetClientNew(sock, NULL));

And you can get rid of both the 'cleanup' label and the 'ret' variable.


All of this is just me being pedantic though :) I saw that you erased some
'exit' labels in other patches and wanted to point out that this label is
also removable. In the end I believe you can keep this patch just adding
g_autofree and removing VIR_FREE calls and, if you want, remove these labels
in a separated patch.



Thanks,



DHB



> 
> Best Regards,
> 


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

On Fri, Dec 20, 2019 at 12:30 AM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 12/19/19 7:17 PM, Fabiano Fidêncio wrote:
> > On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza
> > <danielhb413@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
> >>> Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> >>> ---
> >>>    src/rpc/virnetclient.c | 18 ++++++------------
> >>>    1 file changed, 6 insertions(+), 12 deletions(-)
> >>>
> >> [...]
> >>
> >>>        if (knownHostsPath) {
> >>> @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
> >>>            goto cleanup;
> >>>
> >>>     cleanup:
> >>> -    VIR_FREE(command);
> >>> -    VIR_FREE(privkey);
> >>> -    VIR_FREE(knownhosts);
> >>> -    VIR_FREE(homedir);
> >>> -    VIR_FREE(confdir);
> >>> -    VIR_FREE(nc);
> >>>        return ret;
> >>>
> >>
> >> This will leave a now unneeded 'cleanup' label:
> >>
> >>    cleanup:
> >>       return ret;
> >>
> >>
> >> In a quick glance at the patch series I noticed that Patch 41 also leaves
> >> an unused 'cleanup' label as well after the changes.
> >
> > Daniel,
> >
> > Please, take a look at the no_memory label. It calls the cleanup one.
>
> I am afraid I wasn't clear. By 'unneeded' I mean a label that is a simply 'return'
> call. 'cleanup' is doing no cleanups anymore. So this this guy:
>
>   no_memory:
>      virReportOOMError();
>      goto cleanup;
>
> turns into:
>
>   no_memory:
>      virReportOOMError();
>      return ret;
>
>
> And in fact, since ret is initialized with NULL, and it will never be set before
> a no_memory jump, you can even do:
>
>   no_memory:
>      virReportOOMError();
>      return NULL;
>
> All 'cleanup' jumps can be changed to 'return NULL' and the line that sets 'ret'
> can turn into
>
>     return virNetClientNew(sock, NULL));
>
> And you can get rid of both the 'cleanup' label and the 'ret' variable.
>
>
> All of this is just me being pedantic though :) I saw that you erased some
> 'exit' labels in other patches and wanted to point out that this label is
> also removable. In the end I believe you can keep this patch just adding
> g_autofree and removing VIR_FREE calls and, if you want, remove these labels
> in a separated patch.

I see. I'll approach this in a different series, tho.

Best Regards,
-- 
Fabiano Fidêncio


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