Re: [libvirt] [PATCH 03/42] rpc: Use g_autofree on virNetClientNewLibSSH2()

On 12/19/19 7:17 PM, Fabiano Fidêncio wrote: > On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza > <danielhb413(a)gmail.com&gt; wrote: >> >> >> >> On 12/19/19 7:04 AM, Fabiano Fidêncio wrote: >>> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt; >>> --- >>> 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.