[virt-tools-list] libosinfo - api revision with glib/gobject

[Daniel P. Berrange]

On Mon, Feb 22, 2010 at 02:56:48PM -0500, Arjun Roy wrote:
> A proposed revision of the libosinfo API follows.
> 
> Modifications made due to suggestions by Dan Berrange in
> https://www.redhat.com/archives/virt-tools-list/2010-February/msg00013.html
> and
> https://www.redhat.com/archives/virt-tools-list/2010-February/msg00000.html
> 
> The biggest change is the conversion of the API to use GLib for types and error
> handling. The top level types of hypervisor, device and operating system now
> each derive from GObject. GError is used for error handling; and any method
> returning a list previously now either returns a GPtrArray (when returning a
> list of either of the three top main types) or returns a GArray (when returning
> other values such as strings). Any C datatype has been replaced with its glib
> version.
> 
> A couple of methods regarding lists and filtering have been added; specifically,
> a method to return a new list as the result of applying a filter to a current
> list, and a method to return a new list as the intersection of two existing
> lists.
> 
> Any comments before porting the API over would be appreciated. A full listing
> of the API follows.

The header file description that follows is not really looking like a normal
GLib/GObject API, since it is grouping & naming things based on a functional
grouping, instead of based on object classes. In particular for easily
generating / binding other languages, there are certain style / naming
conventions that need to be followed.

First of all, each GObject type should be defined in isolation in its own
header. For the most basic data type with no references to other objects,
the header file would look

    #ifndef __MYLIB_MYOBJECT_H__
    #define __MYLIB_MYOBJECT_H__

    #include <glib-object.h>

    G_BEGIN_DECLS

    #define MYLIB_TYPE_MYOBJECT            (mylib_myobject_get_type ())
    #define MYLIB_MYOBJECT(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), MYLIB_TYPE_MYOBJECT, MylibMyobject))
    #define MYLIB_MYOBJECT_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), MYLIB_TYPE_MYOBJECT, MylibMyobjectClass))
    #define MYLIB_IS_MYOBJECT(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MYLIB_TYPE_MYOBJECT))
    #define MYLIB_IS_MYOBJECT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MYLIB_TYPE_MYOBJECT))
    #define MYLIB_MYOBJECT_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), MYLIB_TYPE_MYOBJECT, MylibMyobjectClass))

    typedef struct _MylibMyobject MylibMyobject;
    typedef struct _MylibMyobjectPrivate MylibMyobjectPrivate;
    typedef struct _MylibMyobjectClass MylibMyobjectClass;

    struct _MylibMyobject
    {
        GObject parent;

        MylibMyobjectPrivate *priv;
    };

    struct _MylibMyobjectClass
    {
        GObjectClass parent_class;
    };

    GType mylib_myobject_get_type(void) G_GNUC_CONST;

    MylibMyobject *mylib_myobject_new(void);

    G_END_DECLS

    #endif /* __MYLIB_MYOBJECT_H__ */

The 'new' method is basically a simple wrapper around g_object_new() to make
it quicker to create new objects. You can optionally add constractor params
to the 'new' method, but you can't rely on these being provided if something
calls the g_oibject_new() method directly.

The style convention is that the typename is declared with initial Caps
and has a short library name prefix, followed by typename. The API methods
for a particular object should also always use the library name prefix,
followed by typename, but all in lowercase and separated by underscore.
The 'new' method can never fail, so if there's something involved like
loading a file, you'd have a separate method for that.

The main data type & class structs have to be in the public header file,
but typically no data fields are kept in the public struct definition,
except for a 'priv' pointer to an private data structure only defined
in the .c file

> /*******************************************************************************
> 
>                    Manipulating the library
> 
> ******************************************************************************/
> 
> osi_lib_t * osi_get_lib_handle(const gchar * data_dir, GError ** error);
> gint osi_init_lib(osi_lib_t * lib, GError ** error);
> gint osi_close_lib(osi_lib_t * lib, GError ** error);
> gint osi_set_lib_param(osi_lib_t * lib, gchar * key, gchar * val, GError ** error);
> gchar * osi_get_lib_param(osi_lib_t lib, gchar * key, GError ** error);

Taking this very simple object as an example, I'd expect something looking
about like

    typedef struct _OSIDatabase OSIDatabase;
    typedef struct _OSIDatabasePrivate OSIDatabasePrivate;
    typedef struct _OSIDatabaseClass OSIDatabaseClass;

    struct _OSIDatabase
    {
        GObject parent;

        OSIDatabasePrivate *priv;
    };

    struct _OSIDatabaseClass
    {
        GObjectClass parent_class;
    };

    GType osi_database_get_type(void) G_GNUC_CONST;

    OSIDatabase *osi_database_new(void);

    gboolean osi_database_load(OSIDatabase *db, const gchar *datadir, GError *error);
    gboolean osi_database_set_param(OSIDatabase *db, const gchar *key, const gchar * val, GError *error);
    const gchar *osi_database_get_param(OSIDatabase *db, const gchar *key, GError *error);


> /*******************************************************************************
> 
>                    Manipulating hypervisors
> 
>  ******************************************************************************/
> 
> GArray * osi_get_all_hypervisor_ids(osi_lib_t * lib, GError ** error);
> 
> osi_hypervisor_t * osi_get_hypervisor_by_id(osi_lib_t * lib, gchar * hypervisor_id, GError ** error);

These two would end up associated with the database object, eg

  GList *osi_database_list_all_hypervisor_ids(OSIDatabase *db);
  OSIHypervisor *osi_database_lookup(OSIDatabase *db, const gchar *hypervisor_id);

I think GList might be better than GArray, if the internal storage for the hypervisor
objects is using a GHash, then you can simply use the g_hash_table_get_keys() method
which returns a GList object.

> gchar * osi_get_hv_id(osi_hypervisor_t * hv, GError ** error);
> 
> GArray * osi_get_hv_device_types(osi_hypervisor_t * hv, GError ** error);
> GArray * osi_get_hv_all_property_keys(osi_hypervisor_t * hv, GError ** error);
> GArray * osi_get_hv_property_all_values(osi_hypervisor_t * hv, gchar * propname, GError ** error);
> gchar * osi_get_hv_property_first_value(osi_hypervisor_t * hv, gchar* propname, GError ** error);

    typedef struct _OSIHypervisor OSIHypervisor;
    typedef struct _OSIHypervisorPrivate OSIHypervisorPrivate;
    typedef struct _OSIHypervisorClass OSIHypervisorClass;

    struct _OSIHypervisor
    {
        GObject parent;

        OSIHypervisorPrivate *priv;
    };

    struct _OSIHypervisorClass
    {
        GObjectClass parent_class;
    };

    GType osi_hypervisor_get_type(void) G_GNUC_CONST;

    OSIHypervisor *osi_hypervisor_new(const gchar *id);

    GList *osi_hypervisor_list_device_types(OSIHypervisor *db, GError *error);
    GList *osi_hypervisor_list_property_keys(OSIHypervisor *db, const gchar *key, GError *error);
    const gchar *osi_hypervisor_get_param(OSIHypervisor *db, const gchar *key, GError *error);


> 
> /*******************************************************************************
> 
>                    Manipulating filters
> 
> ******************************************************************************/
> 
> osi_filter_t * osi_get_filter(osi_lib_t * lib, GError ** error);

This would likely want to be a method against the OSIDatabase object

   OSIFilter *osi_database_get_filter(OSIDatabase *db);

> 
> gint osi_add_filter_constraint(osi_filter_t * filter, gchar * propname, gchar * propval, GError ** error);
> gint osi_add_relation_constraint(osi_filter_t * filter, osi_relationship relationship, osi_os_t * os, GError ** error);
> 
> gint osi_clear_filter_constraint(osi_filter_t * filter, gchar * propname, GError ** error);
> gint osi_clear_relation_constraint(osi_filter_t  *filter, osi_relationship relationship, GError ** error);
> gint osi_clear_all_constraints(osi_filter_t * filter, GError ** error);
> 
> GArray * osi_get_filter_constraint_keys(osi_filter_t * filter, GError ** error);
> gchar * osi_get_filter_constraint_value(osi_filter_t * filter, gchar * propname, GError ** error);
> osi_os_t * osi_get_relationship_constraint_value(osi_filter_t * filter, osi_relationship relationship, GError ** error);

As above there'd want to be the basic object definition, then all these methods
start with the prefix 'osi_filter_'

> 
> /*******************************************************************************
> 
>                    Manipulating Devices
> 
>  ************************************************************************g a GPtrArray, I think we'll want to have a "OSIDeviceList"
data type. I also think the 'filter' object should only be applied to the OSIDeviceList
object directly eg  
 
  OSIDeviceList *osi_device_list_apply_filter(OSIDeviceList *list, OSIFilter *filter);

then these two methods do not need the filter

  OSIDeviceList * osi_hypervisor_devices(OSIHypervisor * hv, gchar * section, GError ** error);
  OSIDeviceList * osi_os_devices(OSIOS * os, gchar * section, GError ** error);

since you can easily apply it by calling the generic method

  OSIDeviceList * osi_device_list_intersect(OSIDeviceList * devices_one, OSIDeviceList * devices_two, GError ** error);
  OSIDevice * osi_device_list_lookup(OSIDeviceList * devices, gint index, GError ** error);


> gchar * osi_device_id(osi_device_t * device, GError ** error);
> osi_device_t * osi_get_device_by_id(osi_lib_t * lib, gchar * device_id, GError ** error);
> 
> GArray * osi_get_all_device_property_keys(osi_device_t * device, GError ** error);
> GArray * osi_get_device_property_all_values(osi_device_t * device, gchar * property, GError ** error);
> gchar * osi_get_device_property_value(osi_device_t * device, gchar * property, GError ** error);
> 
> gchar * osi_get_device_driver(osi_device_t * device, osi_hypervisor_t * hv, osi_os_t * os, gchar * type, GError ** error);
> osi_device_t * osi_get_preferred_device(osi_os_t * os, osi_hypervisor_t * hv, gchar * section, osi_filter_t * filter, GError ** error);

I'm not sure why we need to pass so many objects into this method it would seem we
could achieve this with a combination of earlier method calls

  a = osi_hypervisor_devices
  b = osi_os_devices
  c = osi_device_list_intersect(a, b)
  d = osi_device_list_apply_filter(c)

then just find the prefered device from the final set 'd'

> /*******************************************************************************
> 
>                    Manipulating Operating Systems
> 
>  ******************************************************************************/
> 
> GPtrArray * osi_get_os_list(osi_lib_t * lib, osi_filter_t * filter, GError ** error);
> GPtrArray * osi_os_list_intersect(GPtrArray * os_list_one, GPtrArray * os_list_two, GError ** err);
> GPtrArray * osi_filter_os_list(GPtrArray * os_list, osi_filter_t * filter, GError ** err);
> 
> osi_os_t osi_get_os_by_index(GPtrArray * list, gint index, GError ** error);
> 
> gchar * osi_get_os_id(osi_os_t * os, GError ** error);
> osi_os_t * osi_get_os_by_id(osi_lib_t * lib, gchar * id, GError ** error);
> 
> GArray * osi_get_all_os_property_keys(osi_os_t * os, GError ** error);
> GArray * osi_get_os_property_all_values(osi_os_t * os, gchar * propname, GError ** error);
> gchar * osi_get_os_property_first_value(osi_os_t * os, gchar * propname, GError ** error);
> GArray * osi_unique_property_values(osi_lib_t * lib, gchar * propname, GError ** error);
> 
> osi_os_t * osi_get_related_os(osi_os_t * os, osi_relationship relationship, GError ** error);
> GPtrArray * osi_unique_relationship_values(osi_lib_t * lib, osi_relationship relationship, GError ** error);
>


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|