8:24 a.m.
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> wrote:
> On Tue, Feb 28, 2012 at 08:25:07PM +0200, Zeeshan Ali (Khattak) wrote:
>> From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org>
>>
>> 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.
>> +{
>> + GType type;
>> +
>> + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_DEVICE(config), NULL);
>> +
>> + if (GVIR_CONFIG_IS_DOMAIN_DISK(config))
>> + type = GVIR_TYPE_DOMAIN_DISK;
>> + else if (GVIR_CONFIG_IS_DOMAIN_INTERFACE(config))
>> + type = GVIR_TYPE_DOMAIN_INTERFACE;
>> + else {
>> + g_debug("Unknown device type: %s",
G_OBJECT_TYPE_NAME(config));
>> + return NULL;
>> + }
>
> I'd have a slight preference for something like
>
> switch (G_OBJECT_TYPE(config)) {
> case GVIR_CONFIG_TYPE_DOMAIN_DISK:
> type = GVIR_TYPE_DOMAIN_DISK;
> break;
> ...
>
> but I'm fine with either version.
I would have prefered it too but if any of these classes are
subclassed tomorrow and we forget to update this code, we'll start
loosing config instances we can actually handle.
Ah good point.
>> +
>> + 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.
>> +
>> + g_object_unref (device_config);
>> + }
>> + g_list_free (config_devices);
>
> The individual elements need to be unref'ed too as well as config.
Thats what g_object_unref is doing just above? I think I should name
True, I didn't pay enough attention.
this 'device_configs' to be consistent with
'device_config' and
therefore clear..
I could pretend that's what confused me... :) This would be better indeed
now that you say it.
Christophe