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

Fabiano Fidêncio fidencio at redhat.com
Thu Dec 19 23:34:24 UTC 2019


On Fri, Dec 20, 2019 at 12:30 AM Daniel Henrique Barboza
<danielhb413 at 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 at gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
> >>> Signed-off-by: Fabiano Fidêncio <fidencio at 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





More information about the libvir-list mailing list