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

[Martin Kletzander]

On 09/05/2012 11:27 AM, Michal Privoznik wrote:
> Let users add either files or devices as disks to domains.
> ---
>  libvirt-designer/libvirt-designer-domain.c |  244 ++++++++++++++++++++++++++++
>  libvirt-designer/libvirt-designer-domain.h |    7 +
>  libvirt-designer/libvirt-designer.sym      |    3 +
>  3 files changed, 254 insertions(+), 0 deletions(-)
> 
> diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c
> index a8cabde..20611f2 100644
> --- a/libvirt-designer/libvirt-designer-domain.c
> +++ b/libvirt-designer/libvirt-designer-domain.c
[...]
> @@ -663,3 +670,240 @@ cleanup:
>          g_object_unref(guest);
>      return ret;
>  }
> +
> +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);
> +
> +    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);
> +    ret = g_list_copy(ret);

It's probably OK here...

> +
> +    g_hash_table_destroy(bus_hash);

...and here, but...

> +    return ret;
> +}
> +
[...]
> +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);
> +    ret = osinfo_device_get_bus_type(dev);

...for example here, shouldn't this be checked for the value passed to
the function?  I know osinfo uses asserts for some of these, but that's
hard to say which of these should be checked and what's the right
procedure as lots of programs throw out these.  I personally don't like
these "<gibberish> CRITICAL <gibberish>" lines at all.
But I'll get to that in another patch anyway ;)

Other than that this patch seems decent, even though I didn't do such a
thorough investigation as I do with other patches as this is pretty new
project.

Martin