Re: [libvirt-glib] Add API to redefine an existing domain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]