[libvirt] [PATCH 3/4] VIR_FREE: Avoid doing side work in callees

[Michal Privoznik]

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 at 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