Re: [libvirt] [PATCH] network: Set to NULL after virNetworkDefFree()
On 10/18/2012 10:44 AM, Michal Privoznik wrote:
which frees all allocated memory but doesn't set the passed
pointer to
NULL. Therefore, we must do it ourselves. This is causing actual
libvirtd crash: Basically, when doing 'virsh net-edit' the newDef should
be dropped.
Well, when you're doing virsh net-edit, if the network is active, you
should be updating obj->newDef, and if the network is inactive, you
should be updating obj->def (and in that case, obj->newDef should have
already been NULL). So I'm not sure what you mean by "newDef should be
dropped" here.
And the memory is freed, indeed. However, the pointer is
not set to NULL but kept instead. And the next duo of calls 'virsh
net-start' and 'virsh net-destroy' starts the disaster.
Interesting, I just started hitting crashes this morning in the opposite
order - if I start a network and that start fails, then I try to edit
it, it will crash during the GetXMLDesc() at the beginning of the edit.
I'll retry now with your patch to make sure it also fixes this scenario
(sounds like it will).
However, I think something is still broken in the logic. The way this is
*supposed* to work (and the reason for the "should be unnecessary"
comment you removed) is that newDef should always be NULL it the network
isn't active, and should always be non-null if it is active. Apparently
that rule is being broken (in spite of that, your patch is of course
still correct :-)
The latter one
does the same as 'virsh destroy'; it sees that newDef is nonNULL so it
replaces def with newDef (which has been freed already as said a few
lines above). Therefore any subsequent call accessing def will hit the ground.
---
src/conf/network_conf.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 891d48c..0f7470d 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -260,8 +260,9 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
return -1;
}
} else if (!live) {
- virNetworkDefFree(network->newDef); /* should be unnecessary */
+ virNetworkDefFree(network->newDef);
virNetworkDefFree(network->def);
+ network->newDef = NULL;
network->def = def;
} else {
virReportError(VIR_ERR_OPERATION_INVALID,