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,
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list