On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote:
On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote:
On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote:
On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote:
On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
@@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj) int virSecretObjSaveData(virSecretObjPtr obj) { - char *base64 = NULL; - int ret = -1; + g_autofree char *base64 = NULL;
I'm not a fan of adding another style here. Either use VIR_FREE(), or convert all VIR_FREE to g_autofree first.
I'm aware it will be unavoidable to use the glib auto pointer macro for complex types but we should at least here where it's interchangable have some kind of consistency.
Here I agree with Peter, for this series I would use VIR_FREE() where it's possible and only for glib objects we can use g_autoptr.
But eventually I would like to switch to g_autofree and friends in order to eliminate our specific helpers in favor of glib helpers.
This also brings a question if we should keep our wrappers for glib or use it directly. For example the string functions that we have.
Where any libvirt code just duplicates something that alrady exists, then I think there's no compelling reason to keep it, the best code is code that doesn't exist.
I don't want todo too many big bang replacements though, so I think best to deprecate existing libvirt code and phase it out incrementally in many cases.
Agreed, for now let's keep all the wrappers but eventually we can remove them to make the code cleaner.
Note that we should be able to use VIR_AUTOPTR() instead of g_autoptr() even for objects, by simply registering the same GLib-provided free function with our own macros.
That way we could keep using VIR_AUTO* everywhere and then, when we're ready, mechanically switch everything to g_auto* in one fell swoop, without having any point in time where the two styles are coexisting.
That creates an even bigger conversion later. Such big conversions cause more pain for backports, than doing an incremental conversion at appropriate times. Converting everything to g_autofree right now doesn't give style consistency as we'd still be matching VIR_ALLOC + g_autofree, so I don't see a benefit to a big conversion in 1 step. Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same time, makes more sense stylewise, as then within the scope of a single method we'd be consistent. 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 :|