[libvirt] [libvirt-designer][PATCH v2 2/4] domain: Introduce disk support

[Christophe Fergeau]

On Mon, Sep 10, 2012 at 03:58:26PM +0200, Michal Privoznik wrote:
> +static GList *
> +gvir_designer_domain_get_supported_disk_bus_types(GVirDesignerDomain *design)
> +{
> +    GVirDesignerDomainPrivate *priv = design->priv;
> +    OsinfoDeviceList *dev_list;
> +    GHashTable *bus_hash = g_hash_table_new(g_str_hash, g_str_equal);
> +    GList *ret = NULL;
> +    int i;
> +
> +    dev_list = osinfo_os_get_devices_by_property(priv->os, "class", "block", TRUE);
> +    if (!dev_list)
> +        goto cleanup;
> +
> +    for (i = 0; i < osinfo_list_get_length(OSINFO_LIST(dev_list)); i++) {
> +        OsinfoDevice *dev = OSINFO_DEVICE(osinfo_list_get_nth(OSINFO_LIST(dev_list), i));
> +        const gchar *bus = osinfo_device_get_bus_type(dev);
> +
> +        if (bus)
> +            g_hash_table_add(bus_hash, g_strdup(bus));
> +    }
> +
> +    ret = g_hash_table_get_keys(bus_hash);
> +    if (ret)
> +        ret = g_list_copy(ret);

NULL is a valid list (empty list), so g_list_copy(NULL); is fine.

> +
> +cleanup:
> +    g_hash_table_destroy(bus_hash);
> +    return ret;
> +}
> +
> +static OsinfoDeviceLink *
> +gvir_designer_domain_get_preferred_device(GVirDesignerDomain *design,
> +                                          const char *class,
> +                                          GError **error)
> +{
> +    GVirDesignerDomainPrivate *priv = design->priv;
> +    OsinfoFilter *filter = osinfo_filter_new();
> +    OsinfoDeviceLinkFilter *filter_link = NULL;
> +    OsinfoDeployment *deployment = priv->deployment;
> +    OsinfoDeviceLink *dev_link = NULL;
> +
> +    if (!deployment) {
> +        priv->deployment = deployment = osinfo_db_find_deployment(osinfo_db,
> +                                                                  priv->os,
> +                                                                  priv->platform);
> +        if (!deployment) {
> +            g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0,
> +                        "Unable to find any deployment in libosinfo database");

g_set_error_literal would be slightly better here.

> +            goto cleanup;
> +        }
> +    }
> +
> +    osinfo_filter_add_constraint(filter, "class", class);
> +    filter_link = osinfo_devicelinkfilter_new(filter);
> +    dev_link = osinfo_deployment_get_preferred_device_link(deployment, OSINFO_FILTER(filter_link));
> +
> +cleanup:
> +    if (filter_link)
> +        g_object_unref(filter_link);
> +    if (filter)
> +        g_object_unref(filter);
> +    return dev_link;
> +}
> +
> +static const gchar *
> +gvir_designer_domain_get_preferred_disk_bus_type(GVirDesignerDomain *design,
> +                                                 GError **error)
> +{
> +    OsinfoDevice *dev;
> +    OsinfoDeviceLink *dev_link = gvir_designer_domain_get_preferred_device(design,
> +                                                                           "block",
> +                                                                           error);
> +    const gchar *ret = NULL;
> +
> +    if (!dev_link)
> +        return NULL;
> +
> +    dev = osinfo_devicelink_get_target(dev_link);
> +    if (dev)
> +        ret = osinfo_device_get_bus_type(dev);
> +
> +    return ret;
> +}
> +
> +static gchar *
> +gvir_designer_domain_next_disk_target(GVirDesignerDomain *design,
> +                                      GVirConfigDomainDiskBus bus)
> +{
> +    gchar *ret = NULL;
> +    GVirDesignerDomainPrivate *priv = design->priv;
> +
> +    switch (bus) {
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_IDE:
> +        ret = g_strdup_printf("hd%c", 'a' + priv->ide++);
> +        break;
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_VIRTIO:
> +        ret = g_strdup_printf("vd%c", 'a' + priv->virtio++);
> +        break;
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_SATA:
> +        ret = g_strdup_printf("sd%c", 'a' + priv->sata++);
> +        break;
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_FDC:
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_SCSI:
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_XEN:
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_USB:
> +    case GVIR_CONFIG_DOMAIN_DISK_BUS_UML:
> +    default:
> +        /* not supported yet */
> +        /* XXX should we fallback to ide/virtio? */
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static GVirConfigDomainDisk *
> +gvir_designer_domain_add_disk_full(GVirDesignerDomain *design,
> +                                   GVirConfigDomainDiskType type,
> +                                   const char *path,
> +                                   const char *format,
> +                                   gchar *target,
> +                                   GError **error)
> +{
> +    GVirDesignerDomainPrivate *priv = design->priv;
> +    GVirConfigDomainDisk *disk = NULL;
> +    GVirConfigDomainDiskBus bus;
> +    gchar *target_gen = NULL;
> +    const gchar *bus_str = NULL;
> +    GList *bus_str_list = NULL, *item = NULL;
> +
> +    /* Guess preferred disk bus */
> +    bus_str = gvir_designer_domain_get_preferred_disk_bus_type(design, error);
> +    if (!bus_str) {
> +        /* And fallback if fails */
> +        bus_str_list = gvir_designer_domain_get_supported_disk_bus_types(design);
> +        if (!bus_str_list) {
> +            if (!*error)

I haven't looked at the callers, but in public APIs, passing a NULL
GError** is acceptable, so this test would be better as 'if (error != NULL
&& *error != NULL)' (I always wonder why glib does not provide
g_error_is_set).

> +                g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0,
> +                            "Unable to find any disk bus type");
> +            goto error;
> +        }
> +
> +        item = g_list_first(bus_str_list);
> +        bus_str = item->data;
> +        if (!bus_str)
> +            goto error;

Looks like 'error' will be leaked in we go to error.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120912/d9554ca8/attachment-0001.sig>