6:54 a.m.
I've looked at a few of these, and one thing I've found is that very
often we have a function called somethingSomethingClear(), and:
1) The only places it is ever called will immediately free the memory
of the object as soon as they clear it.
and very possibly
2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but
then some of the other pointers call
bobLoblawFree(def->bobloblaw)
and then don't actually set def->bobloblaw to NULL - so the functions
aren't actually "Clearing", they're "Freeing and then clearing a
few
things, but not everything".
So I'm wondering if it is worthwhile to
A) audit all the *Clear() functions and rename the functions that
don't actually need to clear the contents to be, e.g.
bobLobLawFreeContents(), while also replacing VIR_FREE with g_free().
(this is what I've done in these 5 patches)
or if we should
B) just do the wholesale approach and (as danpb suggested last week)
change all VIR_FREE in *Clear() functions to g_free(), and put a
"memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring
whether or not we actually need that.
(B) would obviously be much faster to get done, and simpler for
everyone to keep track of what's happening, but of course it is less
efficient. Very likely this efficiency is completely meaningless in
the large scheme (even in the medium or small scheme).
(I actually like the idea of 0'ing out *everything*[*] when we're done
with it, extra cycles be damned! I think of the two choices above,
after going through this exercise, I'd say (B) is a more reasonable
choice, but since I tried this, I thought I'd send it and see if
anyone else has an opinion (or different idea)
[*](including all those places I've replaced VIR_FREE with g_free in
the name of "progress?")
Laine Stump (5):
conf: rename and narrow scope of virDomainHostdevDefClear()
conf: rename virDomainHostdevSubsysSCSIClear
conf: replace pointless VIR_FREEs with g_free in
virDomainVideoDefClear()
conf: don't call virDomainDeviceInfoClear from
virDomainDeviceInfoParseXML
conf: replace virDomainDeviceInfoClear with
virDomainDeviceInfoFreeContents
src/conf/device_conf.c | 15 +++-----
src/conf/device_conf.h | 2 +-
src/conf/domain_conf.c | 81 ++++++++++++++++++++--------------------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
5 files changed, 47 insertions(+), 53 deletions(-)
--
2.29.2
6:54 a.m.
New subject: [libvirt RFC PATCH 1/5] conf: rename and narrow scope of virDomainHostdevDefClear()
This function had the name "Clear", but some of its fields were being
cleared, and others were just being freed, but the pointers not
cleared. In fact, all the uses of virDomainHostdevDefClear() ended up
freeing the memory containing the HosdevDef immediately after clearing
it.
So it is acceptable to change the VIR_FREEs in this function to
g_frees, but that would be confusing to future me, who will have
forgotten that a "cleared" hostdevdef is never re-used. In order to
eliminate the confusion, we can rename the function to
virDomainHostdevDefFreeContents(), which is more accurate (doesn't
make false claims).
In the meantime, this function was only ever called from other
functions within the same file, so there is no need for it to be
declared in domain_conf.h, much less be listed among the exported
functions in libvirt_private.syms, so lets just make this renamed
function static for efficiency's sake.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 19 +++++++++++--------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17e0d1ed31..3b9d0da232 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1339,6 +1339,7 @@ static virClassPtr virDomainXMLOptionClass;
static void virDomainObjDispose(void *obj);
static void virDomainXMLOptionDispose(void *obj);
+static void virDomainHostdevDefFreeContents(virDomainHostdevDefPtr def);
static void
virDomainChrSourceDefFormat(virBufferPtr buf,
@@ -2430,7 +2431,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
g_free(def->data.direct.linkdev);
break;
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
- virDomainHostdevDefClear(&def->data.hostdev.def);
+ virDomainHostdevDefFree(&def->data.hostdev.def);
break;
default:
break;
@@ -2531,7 +2532,7 @@ virDomainNetDefFree(virDomainNetDefPtr def)
break;
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
- virDomainHostdevDefClear(&def->data.hostdev.def);
+ virDomainHostdevDefFree(&def->data.hostdev.def);
break;
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -3009,7 +3010,8 @@ virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSIPtr
scsisrc)
}
-void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
+static void
+virDomainHostdevDefFreeContents(virDomainHostdevDefPtr def)
{
if (!def)
return;
@@ -3030,13 +3032,13 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
switch ((virDomainHostdevCapsType) def->source.caps.type) {
case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
- VIR_FREE(def->source.caps.u.storage.block);
+ g_free(def->source.caps.u.storage.block);
break;
case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC:
- VIR_FREE(def->source.caps.u.misc.chardev);
+ g_free(def->source.caps.u.misc.chardev);
break;
case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET:
- VIR_FREE(def->source.caps.u.net.ifname);
+ g_free(def->source.caps.u.net.ifname);
virNetDevIPInfoClear(&def->source.caps.u.net.ip);
break;
case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST:
@@ -3049,7 +3051,7 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
virDomainHostdevSubsysSCSIClear(&def->source.subsys.u.scsi);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
- VIR_FREE(def->source.subsys.u.scsi_host.wwpn);
+ g_free(def->source.subsys.u.scsi_host.wwpn);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
@@ -3061,6 +3063,7 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
}
}
+
void virDomainTPMDefFree(virDomainTPMDefPtr def)
{
if (!def)
@@ -3089,7 +3092,7 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def)
return;
/* free all subordinate objects */
- virDomainHostdevDefClear(def);
+ virDomainHostdevDefFreeContents(def);
/* If there is a parentnet device object, it will handle freeing
* the memory.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e263e6098b..415826134d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3126,7 +3126,6 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVideoDef, virDomainVideoDefFree);
void virDomainVideoDefClear(virDomainVideoDefPtr def);
virDomainHostdevDefPtr virDomainHostdevDefNew(void);
-void virDomainHostdevDefClear(virDomainHostdevDefPtr def);
void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
void virDomainHubDefFree(virDomainHubDefPtr def);
void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 49e665a5f0..e586263a61 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -445,7 +445,6 @@ virDomainGraphicsVNCSharePolicyTypeFromString;
virDomainGraphicsVNCSharePolicyTypeToString;
virDomainHasNet;
virDomainHostdevCapsTypeToString;
-virDomainHostdevDefClear;
virDomainHostdevDefFree;
virDomainHostdevDefNew;
virDomainHostdevFind;
--
2.29.2
6:54 a.m.
New subject: [libvirt RFC PATCH 2/5] conf: rename virDomainHostdevSubsysSCSIClear
This function is only called from one place, and the object being
"cleared" is never again referenced before being freed, so it doesn't
need to be "cleared", just change its name to *FreeContents() so that
we can remove its VIR_FREEs with a "clear" conscience (pun intended).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3b9d0da232..3505d29dbe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2997,13 +2997,13 @@ virDomainHostdevDefNew(void)
static void
-virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSIPtr scsisrc)
+virDomainHostdevSubsysSCSIFreeContents(virDomainHostdevSubsysSCSIPtr scsisrc)
{
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
virObjectUnref(scsisrc->u.iscsi.src);
scsisrc->u.iscsi.src = NULL;
} else {
- VIR_FREE(scsisrc->u.host.adapter);
+ g_free(scsisrc->u.host.adapter);
virObjectUnref(scsisrc->u.host.src);
scsisrc->u.host.src = NULL;
}
@@ -3048,7 +3048,7 @@ virDomainHostdevDefFreeContents(virDomainHostdevDefPtr def)
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
switch ((virDomainHostdevSubsysType) def->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- virDomainHostdevSubsysSCSIClear(&def->source.subsys.u.scsi);
+ virDomainHostdevSubsysSCSIFreeContents(&def->source.subsys.u.scsi);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
g_free(def->source.subsys.u.scsi_host.wwpn);
--
2.29.2
6:54 a.m.
New subject: [libvirt RFC PATCH 3/5] conf: replace pointless VIR_FREEs with g_free in virDomainVideoDefClear()
This function does a memset(0) at the end anyway, so there's no point
in VIR_FREE vs g_free.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3505d29dbe..d8b8ef38f5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2960,13 +2960,13 @@ virDomainVideoDefClear(virDomainVideoDefPtr def)
virDomainDeviceInfoClear(&def->info);
if (def->accel)
- VIR_FREE(def->accel->rendernode);
- VIR_FREE(def->accel);
- VIR_FREE(def->res);
- VIR_FREE(def->virtio);
+ g_free(def->accel->rendernode);
+ g_free(def->accel);
+ g_free(def->res);
+ g_free(def->virtio);
if (def->driver)
- VIR_FREE(def->driver->vhost_user_binary);
- VIR_FREE(def->driver);
+ g_free(def->driver->vhost_user_binary);
+ g_free(def->driver);
virObjectUnref(def->privateData);
memset(def, 0, sizeof(*def));
--
2.29.2
6:54 a.m.
New subject: [libvirt RFC PATCH 4/5] conf: don't call virDomainDeviceInfoClear from virDomainDeviceInfoParseXML
The DeviceInfo object sent to virDomainDeviceInfoParseXML() was
allocated with g_new0() just prior to the call (although in each case
there is some other code in between, none of it touches the
DeviceInfo); pretty clearly, the call to virDomainDeviceInfoClear() at
the top of virDomainDeviceInfoParseXML() was just overly paranoid
"safety code", and is unnecessary.
Likewise, the call to clear the device info on return when there is a
parse failure is also unnecessary, since in every case the caller
immediately frees the object after the failure.
So let's remove these.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8b8ef38f5..f2b00129e1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6533,8 +6533,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *rombar = NULL;
g_autofree char *aliasStr = NULL;
- virDomainDeviceInfoClear(info);
-
cur = node->children;
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
@@ -6611,8 +6609,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
ret = 0;
cleanup:
- if (ret < 0)
- virDomainDeviceInfoClear(info);
return ret;
}
--
2.29.2
6:54 a.m.
New subject: [libvirt RFC PATCH 5/5] conf: replace virDomainDeviceInfoClear with virDomainDeviceInfoFreeContents
In every case where virDomainDeviceInfoClear() is still being called,
the object containing the DeviceInfo is freed immediately thereafter
(yes, I checked them all!). This means we don't need to clear any of
the data in the DeviceInfo, just free memory that it points to.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/device_conf.c | 15 ++++++---------
src/conf/device_conf.h | 2 +-
src/conf/domain_conf.c | 40 ++++++++++++++++++++--------------------
3 files changed, 27 insertions(+), 30 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 714ac50762..743d7f7468 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -83,22 +83,19 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
}
void
-virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+virDomainDeviceInfoFreeContents(virDomainDeviceInfoPtr info)
{
- VIR_FREE(info->alias);
- memset(&info->addr, 0, sizeof(info->addr));
- info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
- VIR_FREE(info->romfile);
- VIR_FREE(info->loadparm);
- info->isolationGroup = 0;
- info->isolationGroupLocked = false;
+ g_free(info->alias);
+ g_free(info->romfile);
+ g_free(info->loadparm);
}
+
void
virDomainDeviceInfoFree(virDomainDeviceInfoPtr info)
{
if (info) {
- virDomainDeviceInfoClear(info);
+ virDomainDeviceInfoFreeContents(info);
g_free(info);
}
}
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a51bdf10ee..444d1b16c6 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -181,7 +181,7 @@ struct _virDomainDeviceInfo {
bool isolationGroupLocked;
};
-void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
+void virDomainDeviceInfoFreeContents(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info);
bool virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a,
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f2b00129e1..b8db94974b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1880,7 +1880,7 @@ void virDomainInputDefFree(virDomainInputDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->source.evdev);
g_free(def->virtio);
g_free(def);
@@ -2210,7 +2210,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def)
g_free(def->domain_name);
g_free(def->blkdeviotune.group_name);
g_free(def->virtio);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
virObjectUnref(def->privateData);
g_free(def);
@@ -2342,7 +2342,7 @@ void virDomainControllerDefFree(virDomainControllerDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->virtio);
g_free(def);
@@ -2408,7 +2408,7 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
virObjectUnref(def->src);
g_free(def->dst);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->virtio);
virObjectUnref(def->privateData);
g_free(def->binary);
@@ -2471,7 +2471,7 @@ virDomainVsockDefFree(virDomainVsockDefPtr vsock)
return;
virObjectUnref(vsock->privateData);
- virDomainDeviceInfoClear(&vsock->info);
+ virDomainDeviceInfoFreeContents(&vsock->info);
g_free(vsock->virtio);
g_free(vsock);
}
@@ -2556,7 +2556,7 @@ virDomainNetDefFree(virDomainNetDefPtr def)
virNetDevIPInfoClear(&def->guestIP);
virNetDevIPInfoClear(&def->hostIP);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->filter);
virHashFree(def->filterparams);
@@ -2806,7 +2806,7 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
}
virObjectUnref(def->source);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -2835,7 +2835,7 @@ void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def)
break;
}
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -2855,7 +2855,7 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
for (i = 0; i < def->ncodecs; i++)
virDomainSoundCodecDefFree(def->codecs[i]);
@@ -2895,7 +2895,7 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->virtio);
g_free(def);
@@ -2906,7 +2906,7 @@ void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -2916,7 +2916,7 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -2926,7 +2926,7 @@ void virDomainShmemDefFree(virDomainShmemDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
virDomainChrSourceDefClear(&def->server.chr);
g_free(def->name);
g_free(def);
@@ -2957,7 +2957,7 @@ virDomainVideoDefClear(virDomainVideoDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
if (def->accel)
g_free(def->accel->rendernode);
@@ -3082,7 +3082,7 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def)
break;
}
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -3106,7 +3106,7 @@ void virDomainHubDefFree(virDomainHubDefPtr def)
if (!def)
return;
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -3116,7 +3116,7 @@ void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def)
return;
virObjectUnref(def->source);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -3143,7 +3143,7 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def)
g_free(def->nvdimmPath);
virBitmapFree(def->sourceNodes);
g_free(def->uuid);
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def);
}
@@ -3352,7 +3352,7 @@ virDomainPanicDefFree(virDomainPanicDefPtr panic)
if (!panic)
return;
- virDomainDeviceInfoClear(&panic->info);
+ virDomainDeviceInfoFreeContents(&panic->info);
g_free(panic);
}
@@ -26579,7 +26579,7 @@ virDomainRNGDefFree(virDomainRNGDefPtr def)
break;
}
- virDomainDeviceInfoClear(&def->info);
+ virDomainDeviceInfoFreeContents(&def->info);
g_free(def->virtio);
g_free(def);
}
--
2.29.2
10:19 a.m.
On 2/12/21 6:54 AM, Laine Stump wrote:
I've looked at a few of these, and one thing I've found is
that very
often we have a function called somethingSomethingClear(), and:
1) The only places it is ever called will immediately free the memory
of the object as soon as they clear it.
and very possibly
2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but
then some of the other pointers call
bobLoblawFree(def->bobloblaw)
and then don't actually set def->bobloblaw to NULL - so the functions
aren't actually "Clearing", they're "Freeing and then clearing a
few
things, but not everything".
So I'm wondering if it is worthwhile to
A) audit all the *Clear() functions and rename the functions that
don't actually need to clear the contents to be, e.g.
bobLobLawFreeContents(), while also replacing VIR_FREE with g_free().
(this is what I've done in these 5 patches)
or if we should
B) just do the wholesale approach and (as danpb suggested last week)
change all VIR_FREE in *Clear() functions to g_free(), and put a
"memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring
whether or not we actually need that.
(B) would obviously be much faster to get done, and simpler for
everyone to keep track of what's happening, but of course it is less
efficient. Very likely this efficiency is completely meaningless in
the large scheme (even in the medium or small scheme).
(I actually like the idea of 0'ing out *everything*[*] when we're done
with it, extra cycles be damned! I think of the two choices above,
after going through this exercise, I'd say (B) is a more reasonable
choice, but since I tried this, I thought I'd send it and see if
anyone else has an opinion (or different idea)
[*](including all those places I've replaced VIR_FREE with g_free in
the name of "progress?")
Laine Stump (5):
conf: rename and narrow scope of virDomainHostdevDefClear()
conf: rename virDomainHostdevSubsysSCSIClear
conf: replace pointless VIR_FREEs with g_free in
virDomainVideoDefClear()
conf: don't call virDomainDeviceInfoClear from
virDomainDeviceInfoParseXML
conf: replace virDomainDeviceInfoClear with
virDomainDeviceInfoFreeContents
src/conf/device_conf.c | 15 +++-----
src/conf/device_conf.h | 2 +-
src/conf/domain_conf.c | 81 ++++++++++++++++++++--------------------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
5 files changed, 47 insertions(+), 53 deletions(-)
I don't like change and thus prefer keeping *Clear() with explicit
memset(0) - if needed, but don't want to stop progress.
Michal
10:43 a.m.
On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
I've looked at a few of these, and one thing I've found is
that very
often we have a function called somethingSomethingClear(), and:
1) The only places it is ever called will immediately free the memory
of the object as soon as they clear it.
and very possibly
2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but
then some of the other pointers call
bobLoblawFree(def->bobloblaw)
and then don't actually set def->bobloblaw to NULL - so the functions
aren't actually "Clearing", they're "Freeing and then clearing a
few
things, but not everything".
One thing I am wondering is whether this is really only used where it makes
sense. As far as I understand, and please correct me if I am way off, the
purpose of the Clear functions is to:
a) provide a way to remove everything from a structure that the current
function cannot recreate (there is a pointer to it somewhere else which
would not be updated) and
b) provide a way to reset a structure so that it can be filled again without
needless reallocation.
I think (b) is obviously pointless, especially lately, so the only reasonable
usage would be for the scenario (a). However, I think I remember this also
being used in places where it would be perfectly fine to free the variable and
recreate it. Maybe it could ease up the decision, at least by eliminating some
of the code, if my hunch is correct.
In my quick search I only found virDomainVideoDefClear to be used in this manner
and I am not convinced that it is worth having this extra function with extra
memset(). Just food for thought.
So I'm wondering if it is worthwhile to
A) audit all the *Clear() functions and rename the functions that
don't actually need to clear the contents to be, e.g.
bobLobLawFreeContents(), while also replacing VIR_FREE with g_free().
(this is what I've done in these 5 patches)
or if we should
B) just do the wholesale approach and (as danpb suggested last week)
change all VIR_FREE in *Clear() functions to g_free(), and put a
"memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring
whether or not we actually need that.
(B) would obviously be much faster to get done, and simpler for
everyone to keep track of what's happening, but of course it is less
efficient. Very likely this efficiency is completely meaningless in
the large scheme (even in the medium or small scheme).
(I actually like the idea of 0'ing out *everything*[*] when we're done
with it, extra cycles be damned! I think of the two choices above,
after going through this exercise, I'd say (B) is a more reasonable
choice, but since I tried this, I thought I'd send it and see if
anyone else has an opinion (or different idea)
[*](including all those places I've replaced VIR_FREE with g_free in
the name of "progress?")
Laine Stump (5):
conf: rename and narrow scope of virDomainHostdevDefClear()
conf: rename virDomainHostdevSubsysSCSIClear
conf: replace pointless VIR_FREEs with g_free in
virDomainVideoDefClear()
conf: don't call virDomainDeviceInfoClear from
virDomainDeviceInfoParseXML
conf: replace virDomainDeviceInfoClear with
virDomainDeviceInfoFreeContents
src/conf/device_conf.c | 15 +++-----
src/conf/device_conf.h | 2 +-
src/conf/domain_conf.c | 81 ++++++++++++++++++++--------------------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
5 files changed, 47 insertions(+), 53 deletions(-)
--
2.29.2
11:07 a.m.
On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
> I've looked at a few of these, and one thing I've found is that very
> often we have a function called somethingSomethingClear(), and:
>
> 1) The only places it is ever called will immediately free the memory
> of the object as soon as they clear it.
>
> and very possibly
>
> 2) It doesn't actually *clear* everything. Some items are cleared via
VIR_FREE(), but then some of the other pointers call
>
> bobLoblawFree(def->bobloblaw)
>
> and then don't actually set def->bobloblaw to NULL - so the functions
> aren't actually "Clearing", they're "Freeing and then
clearing a few
> things, but not everything".
>
One thing I am wondering is whether this is really only used where it makes
sense. As far as I understand, and please correct me if I am way off, the
purpose of the Clear functions is to:
a) provide a way to remove everything from a structure that the current
function cannot recreate (there is a pointer to it somewhere else which
would not be updated) and
b) provide a way to reset a structure so that it can be filled again without
needless reallocation.
I think (b) is obviously pointless, especially lately, so the only reasonable
usage would be for the scenario (a). However, I think I remember this also
being used in places where it would be perfectly fine to free the variable and
recreate it. Maybe it could ease up the decision, at least by eliminating some
of the code, if my hunch is correct.
In my quick search I only found virDomainVideoDefClear to be used in this manner
and I am not convinced that it is worth having this extra function with extra
You could always memset it explicitly, someone might find the code more
readable then. IMO I'd vote for an explicit memset just for the sake of better
security practice (since we'll have to wait a little while for something like
SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
many cycles exactly would be wasted, but IIRC a recent discussion memset can be
optimized away (correct me if I don't remember it well!), so Dan P.B.
suggested to gradually convert to some platform-specific ways on how to
sanitize the memory safely - with that in mind, I'd say we use an explicit
memset in all the functions in question and convert them later?
Erik
11:25 a.m.
On Fri, Feb 12, 2021 at 11:07:21AM +0100, Erik Skultety wrote:
On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
> On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
> > I've looked at a few of these, and one thing I've found is that very
> > often we have a function called somethingSomethingClear(), and:
> >
> > 1) The only places it is ever called will immediately free the memory
> > of the object as soon as they clear it.
> >
> > and very possibly
> >
> > 2) It doesn't actually *clear* everything. Some items are cleared via
VIR_FREE(), but then some of the other pointers call
> >
> > bobLoblawFree(def->bobloblaw)
> >
> > and then don't actually set def->bobloblaw to NULL - so the functions
> > aren't actually "Clearing", they're "Freeing and then
clearing a few
> > things, but not everything".
> >
>
> One thing I am wondering is whether this is really only used where it makes
> sense. As far as I understand, and please correct me if I am way off, the
> purpose of the Clear functions is to:
>
> a) provide a way to remove everything from a structure that the current
> function cannot recreate (there is a pointer to it somewhere else which
> would not be updated) and
>
> b) provide a way to reset a structure so that it can be filled again without
> needless reallocation.
>
> I think (b) is obviously pointless, especially lately, so the only reasonable
> usage would be for the scenario (a). However, I think I remember this also
> being used in places where it would be perfectly fine to free the variable and
> recreate it. Maybe it could ease up the decision, at least by eliminating some
> of the code, if my hunch is correct.
>
> In my quick search I only found virDomainVideoDefClear to be used in this manner
> and I am not convinced that it is worth having this extra function with extra
You could always memset it explicitly, someone might find the code more
readable then. IMO I'd vote for an explicit memset just for the sake of better
security practice (since we'll have to wait a little while for something like
SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
many cycles exactly would be wasted, but IIRC a recent discussion memset can be
optimized away (correct me if I don't remember it well!), so Dan P.B.
suggested to gradually convert to some platform-specific ways on how to
sanitize the memory safely - with that in mind, I'd say we use an explicit
memset in all the functions in question and convert them later?
I only suggest that for places where security is required. ie to scrub
passwords.
If the compiler wants to cull memsets in places unrelated to security
that's fine by me, or at least, not our problem to worry about.
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 :|
7:55 p.m.
On 2/12/21 5:25 AM, Daniel P. Berrangé wrote:
On Fri, Feb 12, 2021 at 11:07:21AM +0100, Erik Skultety wrote:
> On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
>> On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
>>> I've looked at a few of these, and one thing I've found is that very
>>> often we have a function called somethingSomethingClear(), and:
>>>
>>> 1) The only places it is ever called will immediately free the memory
>>> of the object as soon as they clear it.
>>>
>>> and very possibly
>>>
>>> 2) It doesn't actually *clear* everything. Some items are cleared via
VIR_FREE(), but then some of the other pointers call
>>>
>>> bobLoblawFree(def->bobloblaw)
>>>
>>> and then don't actually set def->bobloblaw to NULL - so the functions
>>> aren't actually "Clearing", they're "Freeing and then
clearing a few
>>> things, but not everything".
>>>
>>
>> One thing I am wondering is whether this is really only used where it makes
>> sense. As far as I understand, and please correct me if I am way off, the
>> purpose of the Clear functions is to:
>>
>> a) provide a way to remove everything from a structure that the current
>> function cannot recreate (there is a pointer to it somewhere else which
>> would not be updated) and
>>
>> b) provide a way to reset a structure so that it can be filled again without
>> needless reallocation.
>>
>> I think (b) is obviously pointless, especially lately, so the only reasonable
>> usage would be for the scenario (a). However, I think I remember this also
>> being used in places where it would be perfectly fine to free the variable and
>> recreate it. Maybe it could ease up the decision, at least by eliminating some
>> of the code, if my hunch is correct.
>>
>> In my quick search I only found virDomainVideoDefClear to be used in this manner
>> and I am not convinced that it is worth having this extra function with extra
>
> You could always memset it explicitly, someone might find the code more
> readable then. IMO I'd vote for an explicit memset just for the sake of better
> security practice (since we'll have to wait a little while for something like
> SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
> many cycles exactly would be wasted, but IIRC a recent discussion memset can be
> optimized away (correct me if I don't remember it well!), so Dan P.B.
> suggested to gradually convert to some platform-specific ways on how to
> sanitize the memory safely - with that in mind, I'd say we use an explicit
> memset in all the functions in question and convert them later?
I only suggest that for places where security is required. ie to scrub
passwords.
Yeah, I'm not planning to touch anything that is clearing out passwords
and such. Only the *Clear() functions that currently have the dual
purposes of:
1) freeing memory pointed to by the object in question (and any sub-objects)
2) clearing out the object so that it can be re-used with no side
effects (e.g., pointers NULLed so that subsequent uses believe
(correctly) that nothing is being pointed at, setting counters to 0,
types to ..._NONE, etc.
If the compiler wants to cull memsets in places unrelated to
security
that's fine by me, or at least, not our problem to worry about.
I would hope that the compiler would be smart enough to not optimize it
out if it can't determine 100% that it will never make a difference.
This would mean that, for example, unless a *Clear() function is defined
static, it couldn't optimize out a memset() at the end (because it can't
know what would be done with the object after return).
But if it's going to optimize out a memset, it would likely also
optimize out the "loblaw = NULL;" in the VIR_FREE invocation, so...
(My mind keeps going back to 1994, when I turned on the 80386 "invalid
address faults" bit (forget the exact name) on our router product that
was running 8086 realmode *BSD, and suddenly so many stupid pointer bugs
were immediately revealed )by a segfault) instead of the code just
silently going off into the weeds. And when we started NULLing out
pointers as things were freed we found so many more; the sources of
mysterious problems that customers had been reporting for months were
suddenly obvious. So my subconscious tells me that NULLing out freed
pointers (and the memory they point to) is just "safer", and we're
spending all this time removing that safety; kind of like going through
all the cars in the world to remove their seatbelts because they make
driving less convenient, and airbags offer a similar type of protection...)
12:14 p.m.
On 2/12/21 11:07 AM, Erik Skultety wrote:
On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
> On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
>> I've looked at a few of these, and one thing I've found is that very
>> often we have a function called somethingSomethingClear(), and:
>>
>> 1) The only places it is ever called will immediately free the memory
>> of the object as soon as they clear it.
>>
>> and very possibly
>>
>> 2) It doesn't actually *clear* everything. Some items are cleared via
VIR_FREE(), but then some of the other pointers call
>>
>> bobLoblawFree(def->bobloblaw)
>>
>> and then don't actually set def->bobloblaw to NULL - so the functions
>> aren't actually "Clearing", they're "Freeing and then
clearing a few
>> things, but not everything".
>>
>
> One thing I am wondering is whether this is really only used where it makes
> sense. As far as I understand, and please correct me if I am way off, the
> purpose of the Clear functions is to:
>
> a) provide a way to remove everything from a structure that the current
> function cannot recreate (there is a pointer to it somewhere else which
> would not be updated) and
>
> b) provide a way to reset a structure so that it can be filled again without
> needless reallocation.
>
> I think (b) is obviously pointless, especially lately, so the only reasonable
> usage would be for the scenario (a). However, I think I remember this also
> being used in places where it would be perfectly fine to free the variable and
> recreate it. Maybe it could ease up the decision, at least by eliminating some
> of the code, if my hunch is correct.
>
> In my quick search I only found virDomainVideoDefClear to be used in this manner
> and I am not convinced that it is worth having this extra function with extra
You could always memset it explicitly, someone might find the code more
readable then. IMO I'd vote for an explicit memset just for the sake of better
security practice (since we'll have to wait a little while for something like
SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
many cycles exactly would be wasted, but IIRC a recent discussion memset can be
optimized away (correct me if I don't remember it well!), so Dan P.B.
suggested to gradually convert to some platform-specific ways on how to
sanitize the memory safely - with that in mind, I'd say we use an explicit
memset in all the functions in question and convert them later?
I think one can argue that if there's a memset() called inside a
function that is supposed to clear out a member of a struct and later
the member is tested against NULL, but compiler decides to "optimize"
memset out - it's a compiler bug.
Michal
1:31 p.m.
On Fri, Feb 12, 2021 at 12:14:27PM +0100, Michal Privoznik wrote:
On 2/12/21 11:07 AM, Erik Skultety wrote:
> On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
> > On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
> > > I've looked at a few of these, and one thing I've found is that
very
> > > often we have a function called somethingSomethingClear(), and:
> > >
> > > 1) The only places it is ever called will immediately free the memory
> > > of the object as soon as they clear it.
> > >
> > > and very possibly
> > >
> > > 2) It doesn't actually *clear* everything. Some items are cleared via
VIR_FREE(), but then some of the other pointers call
> > >
> > > bobLoblawFree(def->bobloblaw)
> > >
> > > and then don't actually set def->bobloblaw to NULL - so the
functions
> > > aren't actually "Clearing", they're "Freeing and
then clearing a few
> > > things, but not everything".
> > >
> >
> > One thing I am wondering is whether this is really only used where it makes
> > sense. As far as I understand, and please correct me if I am way off, the
> > purpose of the Clear functions is to:
> >
> > a) provide a way to remove everything from a structure that the current
> > function cannot recreate (there is a pointer to it somewhere else which
> > would not be updated) and
> >
> > b) provide a way to reset a structure so that it can be filled again without
> > needless reallocation.
> >
> > I think (b) is obviously pointless, especially lately, so the only reasonable
> > usage would be for the scenario (a). However, I think I remember this also
> > being used in places where it would be perfectly fine to free the variable and
> > recreate it. Maybe it could ease up the decision, at least by eliminating
some
> > of the code, if my hunch is correct.
> >
> > In my quick search I only found virDomainVideoDefClear to be used in this
manner
> > and I am not convinced that it is worth having this extra function with extra
>
> You could always memset it explicitly, someone might find the code more
> readable then. IMO I'd vote for an explicit memset just for the sake of better
> security practice (since we'll have to wait a little while for something like
> SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
> many cycles exactly would be wasted, but IIRC a recent discussion memset can be
> optimized away (correct me if I don't remember it well!), so Dan P.B.
> suggested to gradually convert to some platform-specific ways on how to
> sanitize the memory safely - with that in mind, I'd say we use an explicit
> memset in all the functions in question and convert them later?
I think one can argue that if there's a memset() called inside a function
that is supposed to clear out a member of a struct and later the member is
tested against NULL, but compiler decides to "optimize" memset out - it's
a
compiler bug.
I agree - it is, especially now that GCC employs a static analyzer it should be
smart and not optimize it away, not sure whether the memset optimization
happened only with GCC or was a generic thing hence I won't try to make any
comments about Clang.
Erik
1316
days inactive
1316
days old
12 comments
5 participants
participants (5)
-
Daniel P. Berrangé -
Erik Skultety -
Laine Stump -
Martin Kletzander -
Michal Privoznik