Re: [libvirt] [libvirt-glib 6/6] Add gvir_domain_get_devices()

On Wed, Feb 29, 2012 at 04:01:40PM +0200, Zeeshan Ali (Khattak) wrote: > On Wed, Feb 29, 2012 at 3:30 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; wrote: > > On Tue, Feb 28, 2012 at 08:25:07PM +0200, Zeeshan Ali (Khattak) wrote: > >> From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org&gt; > >> > >> Currently we only support existing DomainDevice implementations: > >> DomainDisk and DomainInterface. > >> --- > >>  .../libvirt-gobject-domain-device-private.h        |    2 + > >>  libvirt-gobject/libvirt-gobject-domain-device.c    |   21 ++++++++++ > >>  libvirt-gobject/libvirt-gobject-domain.c           |   42 ++++++++++++++++++++ > >>  libvirt-gobject/libvirt-gobject-domain.h           |    3 + > >>  libvirt-gobject/libvirt-gobject.sym                |    1 + > >>  5 files changed, 69 insertions(+), 0 deletions(-) > >> > >> diff --git a/libvirt-gobject/libvirt-gobject-domain-device-private.h b/libvirt-gobject/libvirt-gobject-domain-device-private.h > >> index 72c660e..292a2ac 100644 > >> --- a/libvirt-gobject/libvirt-gobject-domain-device-private.h > >> +++ b/libvirt-gobject/libvirt-gobject-domain-device-private.h > >> @@ -24,6 +24,8 @@ > >> > >>  G_BEGIN_DECLS > >> > >> +G_GNUC_INTERNAL GVirDomainDevice *gvir_domain_device_new(GVirConfigDomainDevice *config, > >> +                                                         GVirDomain *domain); > >>  virDomainPtr gvir_domain_device_get_domain_handle(GVirDomainDevice *self); > >> > >>  G_END_DECLS > >> diff --git a/libvirt-gobject/libvirt-gobject-domain-device.c b/libvirt-gobject/libvirt-gobject-domain-device.c > >> index 85879d2..0ec5dad 100644 > >> --- a/libvirt-gobject/libvirt-gobject-domain-device.c > >> +++ b/libvirt-gobject/libvirt-gobject-domain-device.c > >> @@ -176,3 +176,24 @@ GVirDomain *gvir_domain_device_get_domain(GVirDomainDevice *device) { > >>  GVirConfigDomainDevice *gvir_domain_device_get_config(GVirDomainDevice *device) { > >>      return g_object_ref (device->priv->config); > >>  } > >> + > >> +G_GNUC_INTERNAL GVirDomainDevice *gvir_domain_device_new(GVirConfigDomainDevice *config, > >> +                                                         GVirDomain *domain) > > > > gvir_domain_new_device(GVirDomain *domain, GVirConfigDomainDevice *config) > > maybe ? > > Having the argument that is always non-NULL first feels better to me. > > Who said config is nullable? :) I assumed this because of the lack of g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_DEVICE(config), ...)

It still kind of sounds better to me to say "let's create a device associated with this GVirDomain", ie to have this method operating on a GVirDomain.

> >> + > >> +        device_config = GVIR_CONFIG_DOMAIN_DEVICE(node->data); > >> +        device = gvir_domain_device_new(device_config, domain); > >> +        if (device != NULL) > >> +             ret = g_list_append(ret, device); > > > > slight preference for g_list_prepend (much more efficient when the list is > > big). > > Good point but wouldn't you expect devices to be sorted in the order > they are sorted in the XML? You can always do a g_list_reverse after the fact, and I'd rather we avoid to make such guarantees anyway. If gvir_config_domain_get_devices used g_list_prepend (which I just noticed it's not doing), you'd need to prepend to return things in the order of the XML :) Not that important here, it's just a better practice to use g_list_prepend.