[virt-tools-list] [libosinfo 3/3] Utility function to retrieve device by property

[Zeeshan Ali (Khattak)]

On Thu, Jan 19, 2012 at 6:17 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Thu, Jan 19, 2012 at 06:09:15PM +0200, Zeeshan Ali (Khattak) wrote:
>> On Thu, Jan 19, 2012 at 5:42 PM, Zeeshan Ali (Khattak)
>> <zeeshanak at gnome.org> wrote:
>> > On Thu, Jan 19, 2012 at 5:28 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> >> On Thu, Jan 19, 2012 at 05:22:20PM +0200, Zeeshan Ali (Khattak) wrote:
>> >>> On Thu, Jan 19, 2012 at 11:51 AM, Christophe Fergeau
>> >>> <cfergeau at redhat.com> wrote:
>> >>> > On Thu, Jan 19, 2012 at 02:37:07AM +0200, Zeeshan Ali (Khattak) wrote:
>> >>> >> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
>> >>> >>
>> >>> >> ---
>> >>> >>  osinfo/libosinfo.syms |    1 +
>> >>> >>  osinfo/osinfo_os.c    |   41 +++++++++++++++++++++++++++++++++++++++++
>> >>> >>  osinfo/osinfo_os.h    |    4 ++++
>> >>> >>  3 files changed, 46 insertions(+), 0 deletions(-)
>> >>> >>
>> >>> >> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
>> >>> >> index de1ff18..8e2d929 100644
>> >>> >> --- a/osinfo/libosinfo.syms
>> >>> >> +++ b/osinfo/libosinfo.syms
>> >>> >> @@ -109,6 +109,7 @@ LIBOSINFO_0.0.6 {
>> >>> >>       osinfo_os_new;
>> >>> >>       osinfo_os_get_devices;
>> >>> >>       osinfo_os_get_all_devices;
>> >>> >> +     osinfo_os_get_device_by_property;
>> >>> >>       osinfo_os_get_device_links;
>> >>> >>       osinfo_os_add_device;
>> >>> >>       osinfo_os_get_family;
>> >>> >> diff --git a/osinfo/osinfo_os.c b/osinfo/osinfo_os.c
>> >>> >> index 0facb08..ec7886a 100644
>> >>> >> --- a/osinfo/osinfo_os.c
>> >>> >> +++ b/osinfo/osinfo_os.c
>> >>> >> @@ -238,6 +238,47 @@ OsinfoDeviceList *osinfo_os_get_all_devices(OsinfoOs *os, OsinfoFilter *filter)
>> >>> >>  }
>> >>> >>
>> >>> >>  /**
>> >>> >> + * osinfo_os_get_device_by_property:
>> >>> >> + * @os: an operating system
>> >>> >> + * @property: the property of interest
>> >>> >> + * @value: the required value of property @property
>> >>> >> + * @inherited: Should devices from inherited and cloned OSs be included in the
>> >>> >> + * search.
>> >>> >> + *
>> >>> >> + * A utility function that gets the first device found from the list of devices
>> >>> >> + * @os supports, for which the value of @property is @value.
>> >>> >> + *
>> >>> >> + * Returns: (transfer full): The found device or NULL
>> >>> >> + */
>> >>> >> +OsinfoDevice *osinfo_os_get_device_by_property(OsinfoOs *os,
>> >>> >> +                                               const gchar *property,
>> >>> >> +                                               const gchar *value,
>> >>> >> +                                               gboolean inherited)
>> >>> >> +{
>> >>> >> +    OsinfoDevice *ret;
>> >>> >> +    OsinfoDeviceList *devices;
>> >>> >> +    OsinfoFilter *filter;
>> >>> >> +
>> >>> >> +    filter = osinfo_filter_new();
>> >>> >> +    osinfo_filter_add_constraint(filter, property, value);
>> >>> >> +
>> >>> >> +    if (inherited)
>> >>> >> +        devices = osinfo_os_get_all_devices(os, filter);
>> >>> >> +    else
>> >>> >> +        devices = osinfo_os_get_devices(os, filter);
>> >>> >> +    g_object_unref (filter);
>> >>> >
>> >>> > Don't you get a warning if you don't do g_object_unref(G_OBJECT(filter)); ?
>> >>> > (same comment for the g_object_unref(devices) below)
>> >>>
>> >>> Nope, should I be?
>> >>
>> >> It probably depends on your CFLAGS, but I'm quite sure I got warnings
>> >> in some occasions without the casts.
>> >>
>> >>>
>> >>> >> +
>> >>> >> +    if (osinfo_list_get_length(OSINFO_LIST(devices)) > 0)
>> >>> >> +        ret = OSINFO_DEVICE (osinfo_list_get_nth(OSINFO_LIST(devices), 0));
>> >>> >
>> >>> > I think it would be better to return the full list and let the app decide whether
>> >>> > it wants the first item in the list or more than that.
>> >>>
>> >>> I guess, we can provide another function 'get_devices_by_property'  as well..
>> >>
>> >> What kind of properties do we get? Does it really make sense to only return
>> >> the first one and ignore the other results? I don't know what kind of
>> >> results this function returns, so hardcoding in the library this kind of
>> >> "arbitrarily pick one result, drop the rest" makes me uncomfortable.
>> >
>> > https://bugzilla.gnome.org/show_bug.cgi?id=668229
>>
>> To help you avoid reading Vala code, the usecase is of creating a new
>> VM for a paritcular OS. The sensible thing to do would be to add only
>> devices supported by that OS. It also would usually make sense to add
>> only one device of a particular class/type not all. So IMHO there is
>> good chances that this function will be useful to other apps as well.
>
> I'm not questioning the usefulness of the function you want to add (or a
> variation on it), what I'm questioning is hardcoding the "let's only
> keep the first device in the list" in the library, it feels cleaner to
> return a list and let the application pick only the first device if it
> wants to.

Thats why I said, we can provide both utility function so app can
choose the one it needs. Even without this other function that returns
all devices, we are still not forcing apps to use this.

About choice of first device, devices are loaded from OS to top
(recursing upwards in hierarch) so the first device you get is the
best one supported by the OS in question (or at least it is supposed
to be).

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124