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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
