[libvirt] [PATCH v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Daniel P. Berrangé]

On Tue, Jul 24, 2018 at 10:46:54PM +0530, Sukrit Bhatnagar wrote:
> On Tue, 24 Jul 2018 at 11:54, Erik Skultety <eskultet at redhat.com> wrote:
> >
> > On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
> > > On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet at redhat.com> wrote:
> > > >
> > > > On Sat, Jul 21, 2018 at 05:36:47PM +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 at gmail.com>
> > > > > ---
> > > > >  src/util/virfirewall.c | 16 +++++-----------
> > > > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > > > > index dfd792f..b4a4d06 100644
> > > > > --- a/src/util/virfirewall.c
> > > > > +++ b/src/util/virfirewall.c
> > > > > @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
> > > > >                                   virFirewallRulePtr rule,
> > > > >                                   const char *fmt, ...)
> > > > >  {
> > > > > -    char *arg;
> > > > > +    VIR_AUTOFREE(char *) arg = NULL;
> > > > >      va_list list;
> > > > >
> > > > >      VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> > > > > @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
> > > > >
> > > > >      va_end(list);
> > > > >
> > > > > -    VIR_FREE(arg);
> > > > >      return;
> > > > >
> > > > >   no_memory:
> > > > >      firewall->err = ENOMEM;
> > > > >      va_end(list);
> > > > > -    VIR_FREE(arg);
> > > >
> > > > There could be an additional patch replacing the no_memory label with 'cleanup'
> > > > with the obvious adjustments.
> > >
> > > There are many functions in virfirewall.c where no_memory is used
> > > instead of cleanup.
> > > So I should change either all of them, or none of them.
> >
> > Yep, so it should be all of them.
> 
> Also, according to libvirt hacking page, no_memory is a standard label [1].
> Should we replace it then?

No, the hacking page is correct - 'no_memory' should be used where the
label is specifically related to OOM handling. That is the case in this
code snippet, as the label is explicitly setting "firewall->err = ENOMEM".
Using "cleanup" here would be misleading, suggesting it was generic
error handling.

> 
> [1]: https://libvirt.org/hacking.html#goto

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|