Re: [libvirt] [libvirt-glib 23/37] Add test for adding a disk device
On Fri, Nov 11, 2011 at 07:05:18PM +0100, Marc-André Lureau wrote:
On Thu, Nov 10, 2011 at 9:33 PM, Christophe Fergeau
<cfergeau(a)redhat.com> wrote:
> devices = g_list_append(devices, disk);
> + gvir_config_domain_set_devices(domain, devices);
> + g_list_free(devices);
> + devices = NULL;
Hmm, I realize this is a bit more tricky. You give up devices element
owner ship but no the container.. I think this is bad, as the list
contains invalid objects after leaving the functions. Insead, the
set_devices () function should ref the element, and the caller should
unref too with g_list_free_full (devices, g_object_unref)
I know this part is a bit messy, the _set_devices function doesn't keep the
device list around so it does not need to get a reference on these. The
reason why there is not a g_list_free_full call in the test program is that
each GVirConfigObject::finalize function will call
xmlFreeDoc(obj->priv->node->doc), but since after calling
gvir_config_domain_set_devices, both GVirConfigDomain and GVirConfigDevice*
will reference the same document, we will get a double free if we unref all
of the objects deriving from GVirConfigObject.
I chose to avoid this issue for now by not unreffing anything in the
example program. Maybe it would be "better" to have the unref here, but to
comment out the xmlFreeDoc() from GVirConfigObject::finalize.
This issue is solved the right way in some future patches by using a
refcounted GVirConfigXmlDoc which wraps xmlDocPtr.
Christophe
Attachments:
- attachment.sig (application/pgp-signature — 198 bytes)