Re: [libvirt] [PATCH 3/4] VIR_FREE: Avoid doing side work in callees
On 15.07.2014 14:58, Ján Tomko wrote:
On 07/15/2014 02:38 PM, Michal Privoznik wrote:
> There are just two places where we rely on the fact that VIR_FREE()
> macro is without any side effects. In the future, this property of the
> macro is going to change, so we need the code to be adjusted to deal
> with argument passed to VIR_FREE() being evaluated multiple times.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/network_conf.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
NACK, this completely removes the side effect. You need to adjust the callers
for that.
Well, the arrays that n<elems> refer to are freed immediately, but I
agree that we should keep the code clean.
IMO we should set n<elems> to 0 in all *Clear functions, not just
virNetworkDNSHostDefClear, after which NULL might be dereferenced based on the
n<elems> variable.
I don't see how that could be possible with current code. The
virNetworkDNSHostDefClear is called only on cleanup paths where
accessing def->names or def->forwarders is not possible anymore.
Jan
Okay, consider this squashed in:
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d60a60e..dcb521f 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -184,6 +184,7 @@ virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def)
for (i = 0; i < def->nnames; i++)
VIR_FREE(def->names[i]);
+ def->nnames = 0;
VIR_FREE(def->names);
}
@@ -204,6 +205,7 @@ virNetworkDNSDefClear(virNetworkDNSDefPtr def)
if (def->forwarders) {
for (i = 0; i < def->nfwds; i++)
VIR_FREE(def->forwarders[i]);
+ def->nfwds = 0;
VIR_FREE(def->forwarders);
}
if (def->txts) {
Michal