On Thu, 2 Aug 2018 at 19:33, Erik Skultety <eskultet@xxxxxxxxxx> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOFREE macro for declaring scalar variables, majority
> > of the VIR_FREE calls can be dropped, which in turn leads to
> > getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@xxxxxxxxx>
> > ---
> > src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------
> > 1 file changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > index e46ac35..bf30d7c 100644
> > --- a/src/util/virnetdevbridge.c
> > +++ b/src/util/virnetdevbridge.c
> > @@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
> > int fd, /* control socket */
> > struct ifreq *ifr) /* pre-filled bridge name */
> > {
> > - char *path = NULL;
> > - int ret = -1;
> > + VIR_AUTOFREE(char *) path = NULL;
> >
> > if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname) < 0)
> > return -1;
> > @@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
> > if (virFileWriteStr(path, valuestr, 0) < 0) {
> > virReportSystemError(errno,
> > _("Unable to set bridge %s %s"), brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > } else {
> > unsigned long paramid;
> > @@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
> > } else {
> > virReportSystemError(EINVAL,
> > _("Unable to set bridge %s %s"), brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > unsigned long args[] = { paramid, value, 0, 0 };
> > ifr->ifr_data = (char*)&args;
> > if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> > virReportSystemError(errno,
> > _("Unable to set bridge %s %s"), brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > }
> >
> > - ret = 0;
> > - cleanup:
> > - VIR_FREE(path);
> > - return ret;
> > + return 0;
> > }
> >
> >
> > @@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
> > const char *paramname, /* sysfs param name */
> > unsigned long *value) /* current value */
> > {
> > - char *path = NULL;
> > + VIR_AUTOFREE(char *) path = NULL;
>
> Referring to my response to previous patch, I'll move ^this at the end of the
> "declare" block (there are a few identical spots across the patch).
>
> ...
>
> > malformed_resp:
> > @@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
> > unsigned int flags, bool isAdd)
> > {
> > int ret = -1;
> > - struct nlmsghdr *resp = NULL;
> > + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> > struct nlmsgerr *err;
> > unsigned int recvbuflen;
> > struct nl_msg *nl_msg;
>
> So, I believe ^this external type can easily be turned into an autoclean
> variant.
>
> > @@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
> > ret = 0;
> > cleanup:
> > nlmsg_free(nl_msg);
>
> So that ^this would be done automatically.
We would need to pass the nlmsg_free function in the cleanup attribute somehow.
So, shall we not use the VIR_AUTO macros and declare it with cleanup attribute
explicitly, or create a new type and a new Free wrapper for it to use
with out macros?
> Otherwise it the patch looks fine.
>
> Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
