Re: [libvirt] [RFC PATCH 3/5] util: storagefile: Introduce helper to free storage source perms
On 06/12/2014 09:02 AM, Peter Krempa wrote:
It will also be reused later.
---
src/util/virstoragefile.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
+static void
+virStoragePermsFree(virStoragePermsPtr def)
+{
+ if (!def)
+ return;
+
+ VIR_FREE(def->label);
+ VIR_FREE(def);
VIR_FREE sets the local variable def to NULL...
+}
+
+
virStorageNetHostDefPtr
virStorageNetHostDefCopy(size_t nhosts,
virStorageNetHostDefPtr hosts)
@@ -1557,10 +1568,7 @@ virStorageSourceClear(virStorageSourcePtr def)
virSecurityDeviceLabelDefFree(def->seclabels[i]);
VIR_FREE(def->seclabels);
}
- if (def->perms) {
- VIR_FREE(def->perms->label);
- VIR_FREE(def->perms);
- }
+ virStoragePermsFree(def->perms);
...but does not change def->perms. If virStorageSourceClear is ever
used in a context where the storage remains live (that is, we are
clearing the object with an intent to reuse it, rather than clearing it
because we are about to free its container), then we risk a stale
pointer being left around. Explicitly setting def->perms = NULL would
avoid that risk. That said...
VIR_FREE(def->timestamps);
virStorageNetHostDefFree(def->nhosts, def->hosts);
...we already have other places that don't bother to clean up stale
pointers, and a quick audit of the source shows that all existing
callers of virStorageSourceClear immediately free the containing object.
If it were worth the paranoia to set things to NULL, we should be doing
it everywhere and not just on the code you are changing here. So:
ACK as-is.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)