Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:58, Erik Skultety wrote: > > On Fri, Aug 03, 2018 at 06:46:28PM +0530, Sukrit Bhatnagar wrote: > > On Fri, 3 Aug 2018 at 18:41, Erik Skultety wrote: > > > > > > On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote: > > > > On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote: > > > > > > > > > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > > > > > > By making use of GNU C's cleanup attribute handled by the > > > > > > VIR_AUTOPTR macro for declaring aggregate pointer variables, > > > > > > majority of the calls to *Free functions can be dropped, which > > > > > > in turn leads to getting rid of most of our cleanup sections. > > > > > > > > > > > > Signed-off-by: Sukrit Bhatnagar > > > > > > --- > > > > > > src/util/virnetdevip.c | 55 > > > > > > +- > > > > > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > > > > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > > > > index 8f1081b..ca206e2 100644 > > > > > > --- a/src/util/virnetdevip.c > > > > > > +++ b/src/util/virnetdevip.c > > > > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > > > > > > } > > > > > > > > > > > > if (!valid) { > > > > > > -virBuffer buf = VIR_BUFFER_INITIALIZER; > > > > > > +VIR_AUTOPTR(virBuffer) buf = NULL; > > > > > > + > > > > > > +if (VIR_ALLOC(buf) < 0) > > > > > > +goto cleanup; > > > > > > > > > > Hmm, this will actually leak memory because @buf is never going to be > > > > > freed, > > > > > worse, we'll assign NULL to it. > > > > > > > > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be > > > > called > > > > when it exits the scope, right? > > > > If I were using virBufferContentAndReset, then it might be the case. > > > > > > How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps > > > @buf. > > > > Got it. I actually made a lot of such changes, have to revert them all. > > Luckily I don't see any of that merged yet, so at least we don't have to hunt > it down in the release. Yes. Most of the changes are in the patches I haven't posted here yet. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 06:46:28PM +0530, Sukrit Bhatnagar wrote: > On Fri, 3 Aug 2018 at 18:41, Erik Skultety wrote: > > > > On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote: > > > On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote: > > > > > > > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > > > > > By making use of GNU C's cleanup attribute handled by the > > > > > VIR_AUTOPTR macro for declaring aggregate pointer variables, > > > > > majority of the calls to *Free functions can be dropped, which > > > > > in turn leads to getting rid of most of our cleanup sections. > > > > > > > > > > Signed-off-by: Sukrit Bhatnagar > > > > > --- > > > > > src/util/virnetdevip.c | 55 > > > > > +- > > > > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > > > index 8f1081b..ca206e2 100644 > > > > > --- a/src/util/virnetdevip.c > > > > > +++ b/src/util/virnetdevip.c > > > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > > > > > } > > > > > > > > > > if (!valid) { > > > > > -virBuffer buf = VIR_BUFFER_INITIALIZER; > > > > > +VIR_AUTOPTR(virBuffer) buf = NULL; > > > > > + > > > > > +if (VIR_ALLOC(buf) < 0) > > > > > +goto cleanup; > > > > > > > > Hmm, this will actually leak memory because @buf is never going to be > > > > freed, > > > > worse, we'll assign NULL to it. > > > > > > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be > > > called > > > when it exits the scope, right? > > > If I were using virBufferContentAndReset, then it might be the case. > > > > How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps > > @buf. > > Got it. I actually made a lot of such changes, have to revert them all. Luckily I don't see any of that merged yet, so at least we don't have to hunt it down in the release. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote: > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > > By making use of GNU C's cleanup attribute handled by the > > VIR_AUTOPTR macro for declaring aggregate pointer variables, > > majority of the calls to *Free functions can be dropped, which > > in turn leads to getting rid of most of our cleanup sections. > > > > Signed-off-by: Sukrit Bhatnagar > > --- > > src/util/virnetdevip.c | 55 > > +- > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > index 8f1081b..ca206e2 100644 > > --- a/src/util/virnetdevip.c > > +++ b/src/util/virnetdevip.c > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > > } > > > > if (!valid) { > > -virBuffer buf = VIR_BUFFER_INITIALIZER; > > +VIR_AUTOPTR(virBuffer) buf = NULL; > > + > > +if (VIR_ALLOC(buf) < 0) > > +goto cleanup; > > Hmm, this will actually leak memory because @buf is never going to be freed, > worse, we'll assign NULL to it. But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called when it exits the scope, right? If I were using virBufferContentAndReset, then it might be the case. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOPTR macro for declaring aggregate pointer variables, > majority of the calls to *Free functions can be dropped, which > in turn leads to getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > --- > src/util/virnetdevip.c | 55 > +- > 1 file changed, 23 insertions(+), 32 deletions(-) > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > index 8f1081b..ca206e2 100644 > --- a/src/util/virnetdevip.c > +++ b/src/util/virnetdevip.c > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > } > > if (!valid) { > -virBuffer buf = VIR_BUFFER_INITIALIZER; > +VIR_AUTOPTR(virBuffer) buf = NULL; > + > +if (VIR_ALLOC(buf) < 0) > +goto cleanup; Hmm, this will actually leak memory because @buf is never going to be freed, worse, we'll assign NULL to it. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:41, Erik Skultety wrote: > > On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote: > > On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote: > > > > > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > > > > By making use of GNU C's cleanup attribute handled by the > > > > VIR_AUTOPTR macro for declaring aggregate pointer variables, > > > > majority of the calls to *Free functions can be dropped, which > > > > in turn leads to getting rid of most of our cleanup sections. > > > > > > > > Signed-off-by: Sukrit Bhatnagar > > > > --- > > > > src/util/virnetdevip.c | 55 > > > > +- > > > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > > index 8f1081b..ca206e2 100644 > > > > --- a/src/util/virnetdevip.c > > > > +++ b/src/util/virnetdevip.c > > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > > > > } > > > > > > > > if (!valid) { > > > > -virBuffer buf = VIR_BUFFER_INITIALIZER; > > > > +VIR_AUTOPTR(virBuffer) buf = NULL; > > > > + > > > > +if (VIR_ALLOC(buf) < 0) > > > > +goto cleanup; > > > > > > Hmm, this will actually leak memory because @buf is never going to be > > > freed, > > > worse, we'll assign NULL to it. > > > > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called > > when it exits the scope, right? > > If I were using virBufferContentAndReset, then it might be the case. > > How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps > @buf. Got it. I actually made a lot of such changes, have to revert them all. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote: > On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote: > > > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote: > > > By making use of GNU C's cleanup attribute handled by the > > > VIR_AUTOPTR macro for declaring aggregate pointer variables, > > > majority of the calls to *Free functions can be dropped, which > > > in turn leads to getting rid of most of our cleanup sections. > > > > > > Signed-off-by: Sukrit Bhatnagar > > > --- > > > src/util/virnetdevip.c | 55 > > > +- > > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > index 8f1081b..ca206e2 100644 > > > --- a/src/util/virnetdevip.c > > > +++ b/src/util/virnetdevip.c > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) > > > } > > > > > > if (!valid) { > > > -virBuffer buf = VIR_BUFFER_INITIALIZER; > > > +VIR_AUTOPTR(virBuffer) buf = NULL; > > > + > > > +if (VIR_ALLOC(buf) < 0) > > > +goto cleanup; > > > > Hmm, this will actually leak memory because @buf is never going to be freed, > > worse, we'll assign NULL to it. > > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called > when it exits the scope, right? > If I were using virBufferContentAndReset, then it might be the case. How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps @buf. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar --- src/util/virnetdevip.c | 55 +- 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 8f1081b..ca206e2 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void) } if (!valid) { -virBuffer buf = VIR_BUFFER_INITIALIZER; +VIR_AUTOPTR(virBuffer) buf = NULL; + +if (VIR_ALLOC(buf) < 0) +goto cleanup; + for (i = 0; i < data.ndevices; i++) { -virBufferAdd(, data.devices[i], -1); +virBufferAdd(buf, data.devices[i], -1); if (i < data.ndevices - 1) -virBufferAddLit(, ", "); +virBufferAddLit(buf, ", "); } virReportError(VIR_ERR_INTERNAL_ERROR, _("Check the host setup: enabling IPv6 forwarding with " "RA routes without accept_ra set to 2 is likely to cause " "routes loss. Interfaces to look at: %s"), - virBufferCurrentContent()); -virBufferFreeAndReset(); + virBufferCurrentContent(buf)); } cleanup: @@ -665,24 +668,23 @@ virNetDevIPAddrAdd(const char *ifname, virSocketAddr *peer, unsigned int prefix) { -virCommandPtr cmd = NULL; +VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) addrstr = NULL; VIR_AUTOFREE(char *) bcaststr = NULL; VIR_AUTOFREE(char *) peerstr = NULL; virSocketAddr broadcast; -int ret = -1; if (!(addrstr = virSocketAddrFormat(addr))) -goto cleanup; +return -1; if (peer && VIR_SOCKET_ADDR_VALID(peer) && !(peerstr = virSocketAddrFormat(peer))) -goto cleanup; +return -1; /* format up a broadcast address if this is IPv4 */ if (!peerstr && ((VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) && ((virSocketAddrBroadcastByPrefix(addr, prefix, ) < 0) || !(bcaststr = virSocketAddrFormat() { -goto cleanup; +return -1; } # ifdef IFCONFIG_PATH @@ -710,12 +712,9 @@ virNetDevIPAddrAdd(const char *ifname, # endif if (virCommandRun(cmd, NULL) < 0) -goto cleanup; +return -1; -ret = 0; - cleanup: -virCommandFree(cmd); -return ret; +return 0; } @@ -724,12 +723,11 @@ virNetDevIPAddrDel(const char *ifname, virSocketAddr *addr, unsigned int prefix) { -virCommandPtr cmd = NULL; +VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) addrstr = NULL; -int ret = -1; if (!(addrstr = virSocketAddrFormat(addr))) -goto cleanup; +return -1; # ifdef IFCONFIG_PATH cmd = virCommandNew(IFCONFIG_PATH); virCommandAddArg(cmd, ifname); @@ -747,12 +745,9 @@ virNetDevIPAddrDel(const char *ifname, # endif if (virCommandRun(cmd, NULL) < 0) -goto cleanup; +return -1; -ret = 0; - cleanup: -virCommandFree(cmd); -return ret; +return 0; } @@ -763,15 +758,14 @@ virNetDevIPRouteAdd(const char *ifname, virSocketAddrPtr gateway, unsigned int metric) { -virCommandPtr cmd = NULL; +VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) addrstr = NULL; VIR_AUTOFREE(char *) gatewaystr = NULL; -int ret = -1; if (!(addrstr = virSocketAddrFormat(addr))) -goto cleanup; +return -1; if (!(gatewaystr = virSocketAddrFormat(gateway))) -goto cleanup; +return -1; cmd = virCommandNew(IP_PATH); virCommandAddArgList(cmd, "route", "add", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); @@ -780,12 +774,9 @@ virNetDevIPRouteAdd(const char *ifname, virCommandAddArgFormat(cmd, "%u", metric); if (virCommandRun(cmd, NULL) < 0) -goto cleanup; +return -1; -ret = 0; - cleanup: -virCommandFree(cmd); -return ret; +return 0; } -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list