Hi Hi Agreed with the comments from teuf + On Fri, Nov 18, 2011 at 8:24 PM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
+void gvir_connection_redefine_domain(GVirConnection *conn, + GVirDomain *domain, + GVirConfigDomain *conf, + GError **err) +{
I would check that the given arguments are correct, != NULL or GVIR_IS_FOO. I would return TRUE on success, and error can be NULL (regular glib convention)
+ const gchar *xml; + virDomainPtr handle; + GVirDomain *dom; + virDomainPtr dom_handle; + GVirConnectionPrivate *priv = conn->priv; + + xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf)); + + g_return_if_fail(xml != NULL); + + g_mutex_lock(priv->lock); + dom = g_hash_table_lookup (priv->domains, + (gpointer)gvir_domain_get_uuid(domain)); + g_mutex_unlock(priv->lock); + g_return_if_fail(dom != NULL); + /* FIXME: Check if config's domain ID is the same as domain passed */
I suppose this is missing Christophe gconfig patches.
+ if (!(handle = virDomainDefineXML(priv->conn, xml))) { + *err = gvir_error_new_literal(GVIR_CONNECTION_ERROR, + 0, + "Failed to redefine domain"); + return NULL; + } +}
And I would verify that handle is == to current domain handle. If not, we probably need to replace it or we need to throw an error. At the minimum it would be nice to leave a comment after verifying that it is safe to ignore return value. (then send an updated patch for ack) regards -- Marc-André Lureau