On Fri, Dec 20, 2019 at 9:14 AM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> On 12/19/19 8:33 PM, Ján Tomko wrote:
> > On Thu, Dec 19, 2019 at 05:22:32PM +0100, Fabiano Fidêncio wrote:
> >> On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
> >>>
> >>> On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
> >>> > Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> >>> > ---
> >>> > src/admin/libvirt-admin.c | 3 +--
> >>> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> s/on/in/ $SUBJECT?
> >>>
> >>> This might be the case for other patches as well.
> >>
> >> Noted.
> >>
> >>>
> >>> One note, I would say it's ok to squash some of the patches from this
> >>> series together, for example all the g_autofree patches per file for
> >>> example.
> >>
> >> I really thought about that. However, it may be misleading as I'm not
> >> touching all the possible conversions to use g_autofree in the other
> >> functions.
> >
> > Well this case is also misleading, since you aren't touching all the
> > possible g_autofree conversions in this functions
>
>
> ACK if you squash this in:
>
> diff --git i/src/admin/libvirt-admin.c w/src/admin/libvirt-admin.c
> index dcbb8ca84d..4099a54854 100644
> --- i/src/admin/libvirt-admin.c
> +++ w/src/admin/libvirt-admin.c
> @@ -100,11 +100,11 @@ static char *
> getSocketPath(virURIPtr uri)
> {
> g_autofree char *rundir = virGetUserRuntimeDirectory();
> - char *sock_path = NULL;
> + g_autofree char *sock_path = NULL;
> size_t i = 0;
>
> if (!uri)
> - goto cleanup;
> + return NULL;
>
>
> for (i = 0; i < uri->paramsCount; i++) {
> @@ -116,7 +116,7 @@ getSocketPath(virURIPtr uri)
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unknown URI parameter '%s'"), param->name);
> - goto error;
> + return NULL;
> }
> }
>
> @@ -127,7 +127,7 @@ getSocketPath(virURIPtr uri)
> if (!uri->scheme) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("No URI scheme specified"));
> - goto error;
> + return NULL;
> }
> if (STREQ(uri->scheme, "libvirtd")) {
> legacy = true;
> @@ -135,7 +135,7 @@ getSocketPath(virURIPtr uri)
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unsupported URI scheme '%s'"),
> uri->scheme);
> - goto error;
> + return NULL;
> }
>
> if (legacy) {
> @@ -152,16 +152,11 @@ getSocketPath(virURIPtr uri)
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Invalid URI path '%s', try '/system'"),
> NULLSTR_EMPTY(uri->path));
> - goto error;
> + return NULL;
> }
> }
>
> - cleanup:
> - return sock_path;
> -
> - error:
> - VIR_FREE(sock_path);
> - goto cleanup;
> + return g_steal_pointer(&sock_path);
> }
>
>
> Michal
Thanks, Michal.
I really haven't thought about using g_steal_pointer in this case.
Taking a "review 101", I'd really expect this to be pointed out as:
- If I missed this, it happened most likely because either I'm not
aware of this or I didn't see the opportunity of doing such change;
- Saying something could be changed but not pointing out what makes
the life of **any** developer harder as the developer has to solve a
puzzle in order to understand the comment;
Please, for the next round, consider (and, at this point, you already
know that) that I'm not as smart as the reviewer and point out what
has to be changed. :-)
Michal went one step further and even provided the diff, it's not
needed, but I'd really expect to be pointed to what's wrong during the
review.
I'll go ahead and push the series.
Best Regards,
--
Fabiano Fidêncio
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list