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@xxxxxxxxx> wrote: > > On Thu, Jan 19, 2012 at 5:28 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> wrote: > >>> > On Thu, Jan 19, 2012 at 02:37:07AM +0200, Zeeshan Ali (Khattak) wrote: > >>> >> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx> > >>> >> > >>> >> --- > >>> >> 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. Christophe
Attachment:
pgpaZR5O4DIog.pgp
Description: PGP signature