Re: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Fri, 2018-08-03 at 09:51 +0200, Erik Skultety wrote:
> > > +void
> > > +virMacAddrFree(virMacAddrPtr addr)
> > > +{
> > > + VIR_FREE(addr);
> > > +}
> >
> > I understand the reason behind this change, however, I don't feel like
this
> > will bring any benefits only because we said that VIR_AUTOFREE should be used
> > with scalar types only, I'd prefer simply using VIR_AUTOFREE here,
CC'ng Pavel
> > to share his opinion.
Sigh, I just realized we already have virPCIDeviceAddressFree which does the
very same thing (possibly more of those) which I missed during the review
of the previous batches. Oh well, we might have to go with this in the end in
order not to cause more confusion.
This kind of change is *very good*. Two main reasons for that:
* Consistency. Whenever you want to free a virSomething, you
should be able to just call virSomethingFree() without having
to look up whether or not virSomething contains dynamically
allocated memory.
* Extensibility. virMacAddrFree() might not do much *right now*,
but there's no guarantee that at some point down the line
virMacAddr won't need to allocate some memory[1], at which
point you'd have to introduce the function anyway as a
prerequisite for your actual changes, in addition of course
to tracking down all uses and convert them from VIR_AUTOFREE()
and VIR_FREE() to VIR_AUTOPTR() and virMacAddrFree().
I'd say that's well worth adding a few extra lines.
[1] Well, virMacAddr specifically probably won't :) But that's
definitely going to be the case for other types (and also
see the first point again :)
--
Andrea Bolognani / Red Hat / Virtualization