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