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