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

Re: [libvirt] [libvirt-designer PATCHv2 3/3] Rework disk bus type handling



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


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