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

2012-09-07 Thread 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


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

2012-09-05 Thread Michal Privoznik
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
@@ -37,6 +37,12 @@ struct _GVirDesignerDomainPrivate
 GVirConfigCapabilities *caps;
 OsinfoOs *os;
 OsinfoPlatform *platform;
+
+OsinfoDeployment *deployment;
+/* next disk targets */
+unsigned int ide;
+unsigned int virtio;
+unsigned int sata;
 };
 
 G_DEFINE_TYPE(GVirDesignerDomain, gvir_designer_domain, G_TYPE_OBJECT);
@@ -134,6 +140,7 @@ static void gvir_designer_domain_finalize(GObject *object)
 g_object_unref(priv-os);
 g_object_unref(priv-platform);
 g_object_unref(priv-caps);
+g_object_unref(priv-deployment);
 
 G_OBJECT_CLASS(gvir_designer_domain_parent_class)-finalize(object);
 }
@@ -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);
+
+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);
+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);
+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