Re: [libvirt-glib PATCHv2 2/5] gobject: Add API to query connection interfaces

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

 



On Mon, Jun 29, 2015 at 03:08:52PM +0100, Zeeshan Ali (Khattak) wrote:
> Add API to query network interfaces from a connection.
> ---
>  libvirt-gobject/libvirt-gobject-connection.c | 226 +++++++++++++++++++++++++++
>  libvirt-gobject/libvirt-gobject-connection.h |   6 +-
>  libvirt-gobject/libvirt-gobject.sym          |   5 +
>  3 files changed, 235 insertions(+), 2 deletions(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c
> index cf073a5..20c43fc 100644
> --- a/libvirt-gobject/libvirt-gobject-connection.c
> +++ b/libvirt-gobject/libvirt-gobject-connection.c
> @@ -44,6 +44,7 @@ struct _GVirConnectionPrivate
>  
>      GHashTable *domains;
>      GHashTable *pools;
> +    GHashTable *interfaces;
>  };
>  
>  G_DEFINE_TYPE(GVirConnection, gvir_connection, G_TYPE_OBJECT);
> @@ -252,6 +253,10 @@ static void gvir_connection_init(GVirConnection *conn)
>                                          g_str_equal,
>                                          NULL,
>                                          g_object_unref);
> +    priv->interfaces = g_hash_table_new_full(g_str_hash,
> +                                             g_str_equal,
> +                                             NULL,
> +                                             g_object_unref);
>  }
>  
>  
> @@ -668,6 +673,11 @@ void gvir_connection_close(GVirConnection *conn)
>          priv->pools = NULL;
>      }
>  
> +    if (priv->interfaces) {
> +        g_hash_table_unref(priv->interfaces);
> +        priv->interfaces = NULL;
> +    }
> +
>      if (priv->conn) {
>          virConnectDomainEventDeregister(priv->conn, domain_event_cb);
>          virConnectClose(priv->conn);
> @@ -1588,6 +1598,222 @@ GVirDomain *gvir_connection_start_domain(GVirConnection *conn,
>  }
>  
>  /**
> + * gvir_connection_fetch_interfaces:
> + * @conn: a #GVirConnection
> + * @cancellable: (allow-none)(transfer none): cancellation object
> + */

Some more detailed API doc here would be nice.

> +gboolean gvir_connection_fetch_interfaces(GVirConnection *conn,
> +                                          GCancellable *cancellable,
> +                                          GError **err)
> +{
> +    GVirConnectionPrivate *priv;
> +    GHashTable *interfaces;
> +    gchar **inactive = NULL;
> +    gint ninactive = 0;
> +    gchar **active = NULL;
> +    gint nactive = 0;
> +    gboolean ret = FALSE;
> +    gint i;
> +    virConnectPtr vconn = NULL;
> +    GError *lerr = NULL;
> +
> +    g_return_val_if_fail(GVIR_IS_CONNECTION(conn), FALSE);
> +    g_return_val_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable),
> +                         FALSE);
> +    g_return_val_if_fail((err == NULL) || (*err == NULL), FALSE);
> +
> +    priv = conn->priv;
> +    g_mutex_lock(priv->lock);
> +    if (!priv->conn) {
> +        g_set_error_literal(err, GVIR_CONNECTION_ERROR,
> +                            0,
> +                            _("Connection is not open"));
> +        g_mutex_unlock(priv->lock);
> +        goto cleanup;
> +    }
> +    vconn = priv->conn;
> +    /* Stop another thread closing the connection just at the minute */
> +    virConnectRef(vconn);
> +    g_mutex_unlock(priv->lock);
> +
> +    if (g_cancellable_set_error_if_cancelled(cancellable, err))
> +        goto cleanup;
> +
> +    active = fetch_list(vconn,
> +                        "Interfaces",
> +                        virConnectNumOfInterfaces,
> +                        virConnectListInterfaces,
> +                        cancellable,
> +                        &nactive,
> +                        &lerr);

Would it be possible to use virConnectListAllInterfaces() rather than
this awkward split between active/inactive interfaces? (which is also
likely to be racy if one interface becomes active between the 2
fetch_list calls).

> +    if (lerr) {
> +        g_propagate_error(err, lerr);
> +        lerr = NULL;
> +        goto cleanup;
> +    }
> +
> +    if (g_cancellable_set_error_if_cancelled(cancellable, err))
> +        goto cleanup;
> +
> +    inactive = fetch_list(vconn,
> +                          "Interfaces",
> +                          virConnectNumOfDefinedInterfaces,
> +                          virConnectListDefinedInterfaces,
> +                          cancellable,
> +                          &ninactive,
> +                          &lerr);
> +    if (lerr) {
> +        g_propagate_error(err, lerr);
> +        lerr = NULL;
> +        goto cleanup;
> +    }
> +
> +    interfaces = g_hash_table_new_full(g_str_hash,
> +                                       g_str_equal,
> +                                       NULL,
> +                                       g_object_unref);
> +
> +    for (i = 0 ; i < nactive ; i++) {
> +        if (g_cancellable_set_error_if_cancelled(cancellable, err))
> +            goto cleanup;
> +
> +        virInterfacePtr viface;
> +        GVirInterface *iface;
> +
> +        viface = virInterfaceLookupByName(vconn, active[i]);
> +        if (!viface)
> +            continue;
> +
> +        iface = GVIR_INTERFACE(g_object_new(GVIR_TYPE_INTERFACE,
> +                                              "handle", viface,
> +                                              NULL));
> +        virInterfaceFree(viface);
> +
> +        g_hash_table_insert(interfaces,
> +                            active[i],
> +                            iface);
> +    }
> +
> +    for (i = 0 ; i < ninactive ; i++) {
> +        if (g_cancellable_set_error_if_cancelled(cancellable, err))
> +            goto cleanup;
> +
> +        virInterfacePtr viface;
> +        GVirInterface *iface;
> +
> +        viface = virInterfaceLookupByName(vconn, inactive[i]);
> +        if (!viface)
> +            continue;
> +
> +        iface = GVIR_INTERFACE(g_object_new(GVIR_TYPE_INTERFACE,
> +                                            "handle", viface,
> +                                            NULL));
> +        virInterfaceFree(viface);
> +
> +        g_hash_table_insert(interfaces,
> +                            active[i],

inactive[i]

> +                            iface);
> +    }
> +
> +    g_mutex_lock(priv->lock);
> +    if (priv->interfaces)
> +        g_hash_table_unref(priv->interfaces);
> +    priv->interfaces = interfaces;
> +    virConnectClose(vconn);

Code you used as a basis has the same bug, but this virConnectClose()
needs to be done in cleanup: as this must match virConnectRef().

> +    g_mutex_unlock(priv->lock);
> +
> +    ret = TRUE;
> +
> +cleanup:
> +    for (i = 0 ; i < nactive ; i++)
> +        g_free(active[i]);
> +    g_free(active);
> +    for (i = 0 ; i < ninactive ; i++)
> +        g_free(inactive[i]);
> +    g_free(inactive);

These g_free() should be free() as they were allocated by libvirt.

> +    return ret;
> +}
> +
> +/**
> + * gvir_connection_get_interfaces:
> + * @conn: a #GVirConnection
> + *
> + * Gets a list of all the network interfaces available through @conn.

I'd make it clearer that these are the host-side network interfaces.

> + *
> + * Return value: (element-type LibvirtGObject.Interface) (transfer full): List
> + * of #GVirInterface. The returned list should be freed with g_list_free(),
> + * after its elements have been unreffed with g_object_unref().
> + */
> +GList *gvir_connection_get_interfaces(GVirConnection *conn)
> +{
> +    GVirConnectionPrivate *priv;
> +    GList *interfaces = NULL;
> +
> +    g_return_val_if_fail(GVIR_IS_CONNECTION(conn), NULL);
> +
> +    priv = conn->priv;
> +    g_mutex_lock(priv->lock);
> +    if (priv->interfaces != NULL) {
> +        interfaces = g_hash_table_get_values(priv->interfaces);
> +        g_list_foreach(interfaces, gvir_domain_ref, NULL);
> +    }
> +    g_mutex_unlock(priv->lock);
> +
> +    return interfaces;
> +}
> +
> +GVirInterface *gvir_connection_get_interface(GVirConnection *conn,
> +                                             const gchar *name)
> +{
> +    GVirConnectionPrivate *priv;
> +    GVirInterface *iface;
> +
> +    g_return_val_if_fail(GVIR_IS_CONNECTION(conn), NULL);
> +    g_return_val_if_fail(name != NULL, NULL);
> +
> +    priv = conn->priv;
> +    g_mutex_lock(priv->lock);
> +    iface = g_hash_table_lookup(priv->interfaces, name);
> +    if (iface)
> +        g_object_ref(iface);
> +    g_mutex_unlock(priv->lock);
> +
> +    return iface;
> +}
> +
> +GVirInterface *gvir_connection_find_interface_by_mac(GVirConnection *conn,
> +                                                     const gchar *mac)
> +{
> +    GVirConnectionPrivate *priv;
> +    GHashTableIter iter;
> +    gpointer key, value;
> +
> +    g_return_val_if_fail(GVIR_IS_CONNECTION(conn), NULL);
> +    g_return_val_if_fail(mac != NULL, NULL);
> +
> +    priv = conn->priv;
> +    g_mutex_lock(priv->lock);
> +    g_hash_table_iter_init(&iter, priv->interfaces);
> +
> +    while (g_hash_table_iter_next(&iter, &key, &value)) {
> +        GVirInterface *iface = value;
> +        const gchar *thismac = gvir_interface_get_mac(iface);
> +
> +        if (thismac == NULL)
> +            continue;
> +
> +        if (strcmp(thismac, mac) == 0) {

Alternatively, could be g_strcmp0(thismac, mac); without the explicit
NULL check, but it's fine your way.

> +            g_object_ref(iface);
> +            g_mutex_unlock(priv->lock);
> +            return iface;
> +        }
> +    }
> +    g_mutex_unlock(priv->lock);
> +
> +    return NULL;
> +}
> +
> +/**
>   * gvir_connection_create_storage_pool:
>   * @conn: a #GVirConnection on which to create the pool
>   * @conf: the configuration for the new storage pool
> diff --git a/libvirt-gobject/libvirt-gobject-connection.h b/libvirt-gobject/libvirt-gobject-connection.h
> index 8bca8d4..77079b6 100644
> --- a/libvirt-gobject/libvirt-gobject-connection.h
> +++ b/libvirt-gobject/libvirt-gobject-connection.h
> @@ -144,14 +144,16 @@ GVirDomain *gvir_connection_start_domain(GVirConnection *conn,
>                                           guint flags,
>                                           GError **err);
>  
> -#if 0
> +gboolean gvir_connection_fetch_interfaces(GVirConnection *conn,
> +                                          GCancellable *cancellable,
> +                                          GError **err);
>  GList *gvir_connection_get_interfaces(GVirConnection *conn);
>  GVirInterface *gvir_connection_get_interface(GVirConnection *conn,
>                                               const gchar *name);
>  GVirInterface *gvir_connection_find_interface_by_mac(GVirConnection *conn,
>                                                       const gchar *macaddr);
>  
> -
> +#if 0
>  GList *gvir_connection_get_networks(GVirConnection *conn);
>  GVirNetwork *gvir_connection_get_network(GVirConnection *conn,
>                                           const gchar *uuid);
> diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym
> index 29c4349..b7ce1d5 100644
> --- a/libvirt-gobject/libvirt-gobject.sym
> +++ b/libvirt-gobject/libvirt-gobject.sym
> @@ -273,6 +273,11 @@ LIBVIRT_GOBJECT_0.2.1 {
>  
>  LIBVIRT_GOBJECT_0.2.2 {
>    global:
> +	gvir_connection_fetch_interfaces;

No async variant ?

Christophe

> +	gvir_connection_find_interface_by_mac;
> +	gvir_connection_get_interface;
> +	gvir_connection_get_interfaces;
> +
>  	gvir_interface_get_mac;
>  } LIBVIRT_GOBJECT_0.2.1;
>  
> -- 
> 2.4.2
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgpxiIGL4K2vU.pgp
Description: PGP signature

--
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]