Hi
Hi
Agreed with the comments from teuf +
On Fri, Nov 18, 2011 at 8:24 PM, Zeeshan Ali (Khattak)
<zeeshanak@xxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list