[libvirt] [libvirt-designer PATCHv2 0/3] Improve disk bus type generation

Here is a new version of this series rebased against master. Christophe

They are useful to tell libvirt-designer about which drivers are install/will be installed in the OS associated with the domain. This in turns allows libvirt-designer code to use these devices when it's making some guesses about what to enable/not enable in the VM being created. --- libvirt-designer/libvirt-designer-domain.c | 79 ++++++++++++++++++++++++++++++ libvirt-designer/libvirt-designer-domain.h | 5 ++ libvirt-designer/libvirt-designer.sym | 3 ++ 3 files changed, 87 insertions(+) diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index 2226d39..f980cb5 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -40,6 +40,8 @@ struct _GVirDesignerDomainPrivate OsinfoPlatform *platform; OsinfoDeployment *deployment; + OsinfoDeviceDriverList *drivers; + /* next disk targets */ unsigned int ide; unsigned int virtio; @@ -61,6 +63,11 @@ gvir_designer_domain_error_quark(void) return g_quark_from_static_string("gvir-designer-domain"); } +static gboolean error_is_set(GError **error) +{ + return ((error != NULL) && (*error != NULL)); +} + enum { PROP_0, PROP_CONFIG, @@ -160,6 +167,8 @@ static void gvir_designer_domain_finalize(GObject *object) g_object_unref(priv->deployment); if (priv->osinfo_db) g_object_unref(priv->osinfo_db); + if (priv->drivers) + g_object_unref(priv->drivers); G_OBJECT_CLASS(gvir_designer_domain_parent_class)->finalize(object); } @@ -235,6 +244,7 @@ static void gvir_designer_domain_init(GVirDesignerDomain *design) priv = design->priv = GVIR_DESIGNER_DOMAIN_GET_PRIVATE(design); priv->config = gvir_config_domain_new(); + priv->drivers = osinfo_device_driverlist_new(); } @@ -1303,3 +1313,72 @@ cleanup: return ret; } + +/** + * gvir_designer_domain_add_driver: + * @design: the domain designer instance + * @driver_id: OsInfo id of the driver to Add + * @error: return location for a #GError, or NULL + * + * Instructs libvirt-designer to assume that the driver identified by + * @driver_id is installed in the guest OS. This means that @design + * can use the device associated to @driver_id if needed. + * + * Returns: (transfer none): TRUE when successfully set, FALSE otherwise. + */ +gboolean gvir_designer_domain_add_driver(GVirDesignerDomain *design, + const char *driver_id, + GError **error) +{ + OsinfoEntity *driver; + OsinfoDeviceDriverList *drivers; + gboolean driver_added = FALSE; + + g_return_val_if_fail(GVIR_DESIGNER_IS_DOMAIN(design), FALSE); + g_return_val_if_fail(driver_id != NULL, FALSE); + g_return_val_if_fail(!error_is_set(error), FALSE); + + if (design->priv->os == NULL) { + g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0, "Unknown OS"); + goto end; + } + + drivers = osinfo_os_get_device_drivers(design->priv->os); + driver = osinfo_list_find_by_id(OSINFO_LIST(drivers), driver_id); + g_return_val_if_fail(OSINFO_IS_DEVICE_DRIVER(driver), FALSE); + if (driver == NULL) { + g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0, + "Unable to find driver %s in OS %s", driver_id, + osinfo_entity_get_id(OSINFO_ENTITY(design->priv->os))); + goto end; + } + + osinfo_list_add(OSINFO_LIST(design->priv->drivers), driver); + driver_added = TRUE; + +end: + return driver_added; +} + + +/** + * gvir_designer_domain_remove_all_drivers: + * @design: the domain designer instance + * @error: return location for a #GError, or NULL + * + * Removes all drivers used in @design. + * + * Returns: (transfer none): TRUE when successfully set, FALSE otherwise. + * @see_also gvir_designer_domain_add_driver() + */ +gboolean gvir_designer_domain_remove_all_drivers(GVirDesignerDomain *design, + GError **error) +{ + g_return_val_if_fail(GVIR_DESIGNER_IS_DOMAIN(design), FALSE); + g_return_val_if_fail(!error_is_set(error), FALSE); + + g_object_unref(design->priv->drivers); + design->priv->drivers = NULL; + + return TRUE; +} diff --git a/libvirt-designer/libvirt-designer-domain.h b/libvirt-designer/libvirt-designer-domain.h index 99c280b..c7b0e5c 100644 --- a/libvirt-designer/libvirt-designer-domain.h +++ b/libvirt-designer/libvirt-designer-domain.h @@ -129,6 +129,11 @@ gboolean gvir_designer_domain_setup_resources(GVirDesignerDomain *design, GVirDesignerDomainResources req, GError **error); +gboolean gvir_designer_domain_remove_all_drivers(GVirDesignerDomain *design, + GError **error); +gboolean gvir_designer_domain_add_driver(GVirDesignerDomain *design, + const char *driver_id, + GError **error); G_END_DECLS #endif /* __LIBVIRT_DESIGNER_DOMAIN_H__ */ diff --git a/libvirt-designer/libvirt-designer.sym b/libvirt-designer/libvirt-designer.sym index 7a24f2c..1dab447 100644 --- a/libvirt-designer/libvirt-designer.sym +++ b/libvirt-designer/libvirt-designer.sym @@ -10,6 +10,9 @@ LIBVIRT_DESIGNER_0.0.1 { gvir_designer_domain_get_platform; gvir_designer_domain_get_capabilities; + gvir_designer_domain_add_driver; + gvir_designer_domain_remove_all_drivers; + gvir_designer_domain_add_cdrom_file; gvir_designer_domain_add_cdrom_device; gvir_designer_domain_add_disk_file; -- 1.8.1.4

On 18.04.2013 18:08, Christophe Fergeau wrote:
They are useful to tell libvirt-designer about which drivers are install/will be installed in the OS associated with the domain. This in turns allows libvirt-designer code to use these devices when it's making some guesses about what to enable/not enable in the VM being created. --- libvirt-designer/libvirt-designer-domain.c | 79 ++++++++++++++++++++++++++++++ libvirt-designer/libvirt-designer-domain.h | 5 ++ libvirt-designer/libvirt-designer.sym | 3 ++ 3 files changed, 87 insertions(+)
diff --git a/libvirt-designer/libvirt-designer.sym b/libvirt-designer/libvirt-designer.sym index 7a24f2c..1dab447 100644 --- a/libvirt-designer/libvirt-designer.sym +++ b/libvirt-designer/libvirt-designer.sym @@ -10,6 +10,9 @@ LIBVIRT_DESIGNER_0.0.1 { gvir_designer_domain_get_platform; gvir_designer_domain_get_capabilities;
+ gvir_designer_domain_add_driver; + gvir_designer_domain_remove_all_drivers; + gvir_designer_domain_add_cdrom_file; gvir_designer_domain_add_cdrom_device; gvir_designer_domain_add_disk_file;
I just realized, shouldn't these new symbols go to into new section LIBVIRT_DESIGNER_0.0.2?

On Fri, Apr 19, 2013 at 11:46:18AM +0200, Michal Privoznik wrote:
On 18.04.2013 18:08, Christophe Fergeau wrote:
They are useful to tell libvirt-designer about which drivers are install/will be installed in the OS associated with the domain. This in turns allows libvirt-designer code to use these devices when it's making some guesses about what to enable/not enable in the VM being created. --- libvirt-designer/libvirt-designer-domain.c | 79 ++++++++++++++++++++++++++++++ libvirt-designer/libvirt-designer-domain.h | 5 ++ libvirt-designer/libvirt-designer.sym | 3 ++ 3 files changed, 87 insertions(+)
diff --git a/libvirt-designer/libvirt-designer.sym b/libvirt-designer/libvirt-designer.sym index 7a24f2c..1dab447 100644 --- a/libvirt-designer/libvirt-designer.sym +++ b/libvirt-designer/libvirt-designer.sym @@ -10,6 +10,9 @@ LIBVIRT_DESIGNER_0.0.1 { gvir_designer_domain_get_platform; gvir_designer_domain_get_capabilities;
+ gvir_designer_domain_add_driver; + gvir_designer_domain_remove_all_drivers; + gvir_designer_domain_add_cdrom_file; gvir_designer_domain_add_cdrom_device; gvir_designer_domain_add_disk_file;
I just realized, shouldn't these new symbols go to into new section LIBVIRT_DESIGNER_0.0.2?
IIRC, we explicitly broke ABI, so we should be incrementing the soname and moving all symbols to a new version block Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This method gathers the list of devices supported by the hypervisor, and intersects this list with the list of devices supported by the OS, natively or using a driver (added with gvir_designer_domain_add_driver()). The lists can be filtered if needed. This commit changes gvir_designer_domain_get_supported_disk_bus_types() to make use of that new helper. This will slightly change its behaviour as before this commit, it will consider any block devices from GVirDesignerDomain::os, while after this commit, it will only consider block devices from GVirDesignerDomain::os that are supported by GVirDesignerDomain::platform. This will cause a change for example for OSes that only list virtio-block as a supported block device, such as Fedora as described in libosinfo v0.2.6-9-g7a8deb4 --- configure.ac | 2 +- libvirt-designer/libvirt-designer-domain.c | 82 +++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index c999826..f03e36d 100644 --- a/configure.ac +++ b/configure.ac @@ -10,7 +10,7 @@ AC_CANONICAL_HOST AM_SILENT_RULES([yes]) -LIBOSINFO_REQUIRED=0.2.3 +LIBOSINFO_REQUIRED=0.2.6 LIBVIRT_GCONFIG_REQUIRED=0.0.9 LIBVIRT_GOBJECT_REQUIRED=0.1.3 GOBJECT_INTROSPECTION_REQUIRED=0.10.8 diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index f980cb5..f959215 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -237,6 +237,80 @@ static void gvir_designer_domain_class_init(GVirDesignerDomainClass *klass) } +static OsinfoDeviceList * +gvir_designer_domain_get_devices_from_drivers(GVirDesignerDomain *design, + OsinfoFilter *filter) +{ + GVirDesignerDomainPrivate *priv = design->priv; + OsinfoDeviceList *devices; + unsigned int i; + + + devices = osinfo_devicelist_new(); + + for (i = 0; i < osinfo_list_get_length(OSINFO_LIST(priv->drivers)); i++) { + OsinfoDeviceDriver *driver; + OsinfoDeviceList *driver_devices; + + driver = OSINFO_DEVICE_DRIVER(osinfo_list_get_nth(OSINFO_LIST(priv->drivers), i)); + driver_devices = osinfo_device_driver_get_devices(driver); + osinfo_list_add_filtered(OSINFO_LIST(devices), + OSINFO_LIST(driver_devices), + filter); + } + + return devices; +} + + +/* Gets the list of devices matching filter that are natively supported + * by (OS) and (platform), or that are supported by (OS with a driver) and + * (platform). + * Drivers are added through gvir_designer_domain_add_driver() + */ +static OsinfoDeviceList * +gvir_designer_domain_get_supported_devices(GVirDesignerDomain *design, + OsinfoFilter *filter) +{ + GVirDesignerDomainPrivate *priv = design->priv; + OsinfoDeviceList *os_devices; + OsinfoDeviceList *platform_devices; + OsinfoDeviceList *driver_devices; + OsinfoDeviceList *devices; + + os_devices = osinfo_os_get_all_devices(priv->os, filter); + platform_devices = osinfo_platform_get_all_devices(priv->platform, filter); + driver_devices = gvir_designer_domain_get_devices_from_drivers(design, filter); + + devices = osinfo_devicelist_new(); + + if (platform_devices == NULL) + goto end; + + if (os_devices != NULL) + osinfo_list_add_intersection(OSINFO_LIST(devices), + OSINFO_LIST(os_devices), + OSINFO_LIST(platform_devices)); + + if (driver_devices != NULL) + osinfo_list_add_intersection(OSINFO_LIST(devices), + OSINFO_LIST(driver_devices), + OSINFO_LIST(platform_devices)); + +end: + if (os_devices != NULL) + g_object_unref(os_devices); + + if (platform_devices != NULL) + g_object_unref(platform_devices); + + if (driver_devices != NULL) + g_object_unref(driver_devices); + + return devices; +} + + static void gvir_designer_domain_init(GVirDesignerDomain *design) { GVirDesignerDomainPrivate *priv; @@ -724,13 +798,15 @@ cleanup: static GList * gvir_designer_domain_get_supported_disk_bus_types(GVirDesignerDomain *design) { - GVirDesignerDomainPrivate *priv = design->priv; OsinfoDeviceList *dev_list; + OsinfoFilter *filter = NULL; GHashTable *bus_hash = g_hash_table_new(g_str_hash, g_str_equal); GList *ret = NULL; GList *devs = NULL, *dev_iterator; - dev_list = osinfo_os_get_devices_by_property(priv->os, "class", "block", TRUE); + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, OSINFO_DEVICE_PROP_CLASS, "block"); + dev_list = gvir_designer_domain_get_supported_devices(design, filter); if (!dev_list) goto cleanup; @@ -750,6 +826,8 @@ cleanup: g_list_free(devs); if (dev_list != NULL) g_object_unref(G_OBJECT(dev_list)); + if (filter != NULL) + g_object_unref(G_OBJECT(filter)); g_hash_table_destroy(bus_hash); return ret; } -- 1.8.1.4

On 18.04.2013 18:08, Christophe Fergeau wrote:
This method gathers the list of devices supported by the hypervisor, and intersects this list with the list of devices supported by the OS, natively or using a driver (added with gvir_designer_domain_add_driver()). The lists can be filtered if needed.
This commit changes gvir_designer_domain_get_supported_disk_bus_types() to make use of that new helper. This will slightly change its behaviour as before this commit, it will consider any block devices from GVirDesignerDomain::os, while after this commit, it will only consider block devices from GVirDesignerDomain::os that are supported by GVirDesignerDomain::platform.
This will cause a change for example for OSes that only list virtio-block as a supported block device, such as Fedora as described in libosinfo v0.2.6-9-g7a8deb4 --- configure.ac | 2 +- libvirt-designer/libvirt-designer-domain.c | 82 +++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac index c999826..f03e36d 100644 --- a/configure.ac +++ b/configure.ac @@ -10,7 +10,7 @@ AC_CANONICAL_HOST
AM_SILENT_RULES([yes])
-LIBOSINFO_REQUIRED=0.2.3 +LIBOSINFO_REQUIRED=0.2.6 LIBVIRT_GCONFIG_REQUIRED=0.0.9 LIBVIRT_GOBJECT_REQUIRED=0.1.3 GOBJECT_INTROSPECTION_REQUIRED=0.10.8
The requirements in README should be updated with this change as well. And yes - they weren't updated previously :) ACK if you update the README. Michal

On Fri, Apr 19, 2013 at 11:46:22AM +0200, Michal Privoznik wrote:
--- a/configure.ac +++ b/configure.ac @@ -10,7 +10,7 @@ AC_CANONICAL_HOST
AM_SILENT_RULES([yes])
-LIBOSINFO_REQUIRED=0.2.3 +LIBOSINFO_REQUIRED=0.2.6 LIBVIRT_GCONFIG_REQUIRED=0.0.9 LIBVIRT_GOBJECT_REQUIRED=0.1.3 GOBJECT_INTROSPECTION_REQUIRED=0.10.8
The requirements in README should be updated with this change as well. And yes - they weren't updated previously :)
ACK if you update the README.
I've just sent a series making sure README and configure.ac always use the same minimum versions as this will scale better. I won't be changing the README in this patch if the other series gets ACKed. Christophe

The current handling of bus types has some issues: - it assumes that if the design uses a disk controller hanging off a PCI bus, then it can use virtio, which is not true for Windows for example unless an additional driver is installed - it checks for "ide", "sata", "virtio" bus names, but they are not used in libosinfo, and "sata is not mentioned in libosinfo.rng - if the bus type could not determined, falling back to an IDE bus should be a safe guess This commit changes the code to guessing the best disk controller to use, and then derives the bus type from it when needed. One limitation of this approach is that we are currently limited to virtio or IDE as libosinfo is not expressive enough for us to tell if a given disk controller is IDE/SATA/SCSI/... One way of making this distinction possible would be to attach the PCI subclass to libosinfo device descriptions as this contains the information we need. --- libvirt-designer/libvirt-designer-domain.c | 185 ++++++++++++++--------------- 1 file changed, 91 insertions(+), 94 deletions(-) diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index f959215..01e48ae 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -23,6 +23,7 @@ */ #include <config.h> +#include <string.h> #include <sys/utsname.h> #include "libvirt-designer/libvirt-designer.h" @@ -68,6 +69,8 @@ static gboolean error_is_set(GError **error) return ((error != NULL) && (*error != NULL)); } +static const char GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID[] = "http://pciids.sourceforge.net/v2.2/pci.ids/1af4/1001"; + enum { PROP_0, PROP_CONFIG, @@ -795,44 +798,6 @@ cleanup: } -static GList * -gvir_designer_domain_get_supported_disk_bus_types(GVirDesignerDomain *design) -{ - OsinfoDeviceList *dev_list; - OsinfoFilter *filter = NULL; - GHashTable *bus_hash = g_hash_table_new(g_str_hash, g_str_equal); - GList *ret = NULL; - GList *devs = NULL, *dev_iterator; - - filter = osinfo_filter_new(); - osinfo_filter_add_constraint(filter, OSINFO_DEVICE_PROP_CLASS, "block"); - dev_list = gvir_designer_domain_get_supported_devices(design, filter); - if (!dev_list) - goto cleanup; - - devs = osinfo_list_get_elements(OSINFO_LIST(dev_list)); - for (dev_iterator = devs; dev_iterator; dev_iterator = dev_iterator->next) { - OsinfoDevice *dev = OSINFO_DEVICE(dev_iterator->data); - 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); - ret = g_list_copy(ret); - -cleanup: - g_list_free(devs); - if (dev_list != NULL) - g_object_unref(G_OBJECT(dev_list)); - if (filter != NULL) - g_object_unref(G_OBJECT(filter)); - g_hash_table_destroy(bus_hash); - return ret; -} - - static OsinfoDeviceLink * gvir_designer_domain_get_preferred_device(GVirDesignerDomain *design, const char *class, @@ -873,27 +838,6 @@ cleanup: } -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) @@ -925,6 +869,84 @@ gvir_designer_domain_next_disk_target(GVirDesignerDomain *design, return ret; } +static OsinfoDevice * +gvir_designer_domain_get_preferred_disk_controller(GVirDesignerDomain *design, + GError **error) +{ + OsinfoDevice *dev = NULL; + OsinfoDeviceLink *dev_link = gvir_designer_domain_get_preferred_device(design, + "block", + error); + if (dev_link == NULL) + goto cleanup; + + dev = osinfo_devicelink_get_target(dev_link); + +cleanup: + if (dev_link != NULL) + g_object_unref(dev_link); + return dev; +} + + +static OsinfoDevice * +gvir_designer_domain_get_fallback_disk_controller(GVirDesignerDomain *design, + GError **error) +{ + OsinfoEntity *dev = NULL; + OsinfoDeviceList *devices; + OsinfoFilter *filter; + int virt_type; + + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, OSINFO_DEVICE_PROP_CLASS, "block"); + devices = gvir_designer_domain_get_supported_devices(design, filter); + g_object_unref(G_OBJECT(filter)); + + if ((devices == NULL) || + (osinfo_list_get_length(OSINFO_LIST(devices)) == 0)) { + goto cleanup; + } + + virt_type = gvir_config_domain_get_virt_type(design->priv->config); + if ((virt_type == GVIR_CONFIG_DOMAIN_VIRT_QEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KQEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KVM)) { + /* If using QEMU; we favour using virtio-block */ + OsinfoList *tmp_devices; + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, + OSINFO_ENTITY_PROP_ID, + GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID); + tmp_devices = osinfo_list_new_filtered(OSINFO_LIST(devices), filter); + if ((tmp_devices != NULL) && + (osinfo_list_get_length(OSINFO_LIST(tmp_devices)) > 0)) { + g_object_unref(devices); + devices = OSINFO_DEVICELIST(tmp_devices); + } + g_object_unref(G_OBJECT(filter)); + } + + dev = osinfo_list_get_nth(OSINFO_LIST(devices), 0); + g_object_ref(G_OBJECT(dev)); + g_object_unref(G_OBJECT(devices)); + +cleanup: + return OSINFO_DEVICE(dev); +} + +static GVirConfigDomainDiskBus +gvir_designer_domain_get_bus_type_from_controller(GVirDesignerDomain *design, + OsinfoDevice *controller) +{ + const char *id; + + id = osinfo_entity_get_id(OSINFO_ENTITY(controller)); + if (strcmp(id, GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID) == 0) + return GVIR_CONFIG_DOMAIN_DISK_BUS_VIRTIO; + + return GVIR_CONFIG_DOMAIN_DISK_BUS_IDE; +} static GVirConfigDomainDisk * gvir_designer_domain_add_disk_full(GVirDesignerDomain *design, @@ -939,30 +961,10 @@ gvir_designer_domain_add_disk_full(GVirDesignerDomain *design, GVirConfigDomainDisk *disk = NULL; GVirConfigDomainDiskBus bus; gchar *target_gen = NULL; - const gchar *bus_str = NULL; const char *driver_name; int virt_type; - 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 && !*error) - 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; - } - - g_clear_error(error); + OsinfoDevice *controller; virt_type = gvir_config_domain_get_virt_type(priv->config); switch (virt_type) { @@ -985,32 +987,28 @@ gvir_designer_domain_add_disk_full(GVirDesignerDomain *design, gvir_config_domain_disk_set_driver_name(disk, driver_name); if (format) gvir_config_domain_disk_set_driver_type(disk, format); - if (g_str_equal(bus_str, "ide")) { - bus = GVIR_CONFIG_DOMAIN_DISK_BUS_IDE; - } else if (g_str_equal(bus_str, "virtio") || - g_str_equal(bus_str, "pci")) { - bus = GVIR_CONFIG_DOMAIN_DISK_BUS_VIRTIO; - } else if (g_str_equal(bus_str, "sata")) { - bus = GVIR_CONFIG_DOMAIN_DISK_BUS_SATA; + + controller = gvir_designer_domain_get_preferred_disk_controller(design, NULL); + if (controller == NULL) + controller = gvir_designer_domain_get_fallback_disk_controller(design, NULL); + + if (controller != NULL) { + bus = gvir_designer_domain_get_bus_type_from_controller(design, controller); } else { - g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0, - "unsupported disk bus type '%s'", bus_str); - goto error; + bus = GVIR_CONFIG_DOMAIN_DISK_BUS_IDE; } - gvir_config_domain_disk_set_target_bus(disk, bus); if (!target) { target = target_gen = gvir_designer_domain_next_disk_target(design, bus); if (!target_gen) { g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0, - "unable to generate target name for bus '%s'", bus_str); + "unable to generate target name for bus '%d'", bus); goto error; } } gvir_config_domain_disk_set_target_dev(disk, target); - g_list_free(bus_str_list); g_free(target_gen); gvir_config_domain_add_device(priv->config, GVIR_CONFIG_DOMAIN_DEVICE(disk)); @@ -1019,7 +1017,6 @@ gvir_designer_domain_add_disk_full(GVirDesignerDomain *design, error: g_free(target_gen); - g_list_free(bus_str_list); if (disk) g_object_unref(disk); return NULL; -- 1.8.1.4

On 18.04.2013 18:08, Christophe Fergeau wrote:
The current handling of bus types has some issues: - it assumes that if the design uses a disk controller hanging off a PCI bus, then it can use virtio, which is not true for Windows for example unless an additional driver is installed - it checks for "ide", "sata", "virtio" bus names, but they are not used in libosinfo, and "sata is not mentioned in libosinfo.rng - if the bus type could not determined, falling back to an IDE bus should be a safe guess
This commit changes the code to guessing the best disk controller to use, and then derives the bus type from it when needed. One limitation of this approach is that we are currently limited to virtio or IDE as libosinfo is not expressive enough for us to tell if a given disk controller is IDE/SATA/SCSI/... One way of making this distinction possible would be to attach the PCI subclass to libosinfo device descriptions as this contains the information we need. --- libvirt-designer/libvirt-designer-domain.c | 185 ++++++++++++++--------------- 1 file changed, 91 insertions(+), 94 deletions(-)
diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index f959215..01e48ae 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -23,6 +23,7 @@ */
#include <config.h> +#include <string.h>
This shouldn't be needed [1]
#include <sys/utsname.h>
#include "libvirt-designer/libvirt-designer.h" @@ -68,6 +69,8 @@ static gboolean error_is_set(GError **error) return ((error != NULL) && (*error != NULL)); }
+static const char GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID[] = "http://pciids.sourceforge.net/v2.2/pci.ids/1af4/1001"; + enum { PROP_0, PROP_CONFIG,
@@ -925,6 +869,84 @@ gvir_designer_domain_next_disk_target(GVirDesignerDomain *design, return ret; }
+static OsinfoDevice * +gvir_designer_domain_get_preferred_disk_controller(GVirDesignerDomain *design, + GError **error) +{ + OsinfoDevice *dev = NULL; + OsinfoDeviceLink *dev_link = gvir_designer_domain_get_preferred_device(design, + "block", + error); + if (dev_link == NULL) + goto cleanup; + + dev = osinfo_devicelink_get_target(dev_link); + +cleanup: + if (dev_link != NULL) + g_object_unref(dev_link); + return dev; +} + + +static OsinfoDevice * +gvir_designer_domain_get_fallback_disk_controller(GVirDesignerDomain *design, + GError **error) +{ + OsinfoEntity *dev = NULL; + OsinfoDeviceList *devices; + OsinfoFilter *filter; + int virt_type; + + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, OSINFO_DEVICE_PROP_CLASS, "block"); + devices = gvir_designer_domain_get_supported_devices(design, filter); + g_object_unref(G_OBJECT(filter)); + + if ((devices == NULL) || + (osinfo_list_get_length(OSINFO_LIST(devices)) == 0)) {
No need for enclosing these two conditions in parentheses here ...
+ goto cleanup; + } + + virt_type = gvir_config_domain_get_virt_type(design->priv->config); + if ((virt_type == GVIR_CONFIG_DOMAIN_VIRT_QEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KQEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KVM)) {
... here ...
+ /* If using QEMU; we favour using virtio-block */ + OsinfoList *tmp_devices; + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, + OSINFO_ENTITY_PROP_ID, + GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID); + tmp_devices = osinfo_list_new_filtered(OSINFO_LIST(devices), filter); + if ((tmp_devices != NULL) && + (osinfo_list_get_length(OSINFO_LIST(tmp_devices)) > 0)) {
... and here.
+ g_object_unref(devices); + devices = OSINFO_DEVICELIST(tmp_devices); + } + g_object_unref(G_OBJECT(filter)); + } + + dev = osinfo_list_get_nth(OSINFO_LIST(devices), 0); + g_object_ref(G_OBJECT(dev)); + g_object_unref(G_OBJECT(devices)); + +cleanup: + return OSINFO_DEVICE(dev); +} + +static GVirConfigDomainDiskBus +gvir_designer_domain_get_bus_type_from_controller(GVirDesignerDomain *design, + OsinfoDevice *controller) +{ + const char *id; + + id = osinfo_entity_get_id(OSINFO_ENTITY(controller)); + if (strcmp(id, GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID) == 0)
1: as g_str_equal is preferred over strcmp.
+ return GVIR_CONFIG_DOMAIN_DISK_BUS_VIRTIO; + + return GVIR_CONFIG_DOMAIN_DISK_BUS_IDE; +}
ACK if you address nits pointed out. Michal

On Fri, Apr 19, 2013 at 11:46:21AM +0200, Michal Privoznik wrote:
On 18.04.2013 18:08, Christophe Fergeau wrote:
+static OsinfoDevice * +gvir_designer_domain_get_fallback_disk_controller(GVirDesignerDomain *design, + GError **error) +{ + OsinfoEntity *dev = NULL; + OsinfoDeviceList *devices; + OsinfoFilter *filter; + int virt_type; + + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, OSINFO_DEVICE_PROP_CLASS, "block"); + devices = gvir_designer_domain_get_supported_devices(design, filter); + g_object_unref(G_OBJECT(filter)); + + if ((devices == NULL) || + (osinfo_list_get_length(OSINFO_LIST(devices)) == 0)) {
No need for enclosing these two conditions in parentheses here ...
+ goto cleanup; + } + + virt_type = gvir_config_domain_get_virt_type(design->priv->config); + if ((virt_type == GVIR_CONFIG_DOMAIN_VIRT_QEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KQEMU) || + (virt_type == GVIR_CONFIG_DOMAIN_VIRT_KVM)) {
... here ...
+ /* If using QEMU; we favour using virtio-block */ + OsinfoList *tmp_devices; + filter = osinfo_filter_new(); + osinfo_filter_add_constraint(filter, + OSINFO_ENTITY_PROP_ID, + GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID); + tmp_devices = osinfo_list_new_filtered(OSINFO_LIST(devices), filter); + if ((tmp_devices != NULL) && + (osinfo_list_get_length(OSINFO_LIST(tmp_devices)) > 0)) {
... and here.
I much prefer having parentheses around conditions, this saves me some thinking effort with respect to operator priorities ;) I've removed the () in this patch, and in the sound/video patches from the other series I've sent. Christophe
participants (3)
-
Christophe Fergeau
-
Daniel P. Berrange
-
Michal Privoznik