On Wed, Feb 29, 2012 at 2:57 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> On Tue, Feb 28, 2012 at 08:25:02PM +0200, Zeeshan Ali (Khattak) wrote:
>> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx>
>>
>> ---
>> libvirt-gconfig/libvirt-gconfig-domain-interface.c | 35 ++++++++++++++++++++
>> libvirt-gconfig/libvirt-gconfig-domain-interface.h | 4 ++
>> libvirt-gconfig/libvirt-gconfig.sym | 4 ++
>> 3 files changed, 43 insertions(+), 0 deletions(-)
>>
>> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-interface.c b/libvirt-gconfig/libvirt-gconfig-domain-interface.c
>> index 85cc194..61d35bd 100644
>> --- a/libvirt-gconfig/libvirt-gconfig-domain-interface.c
>> +++ b/libvirt-gconfig/libvirt-gconfig-domain-interface.c
>> @@ -96,6 +96,41 @@ void gvir_config_domain_interface_set_model(GVirConfigDomainInterface *interface
>> "model", "type", model);
>> }
>>
>> +const char *gvir_config_domain_interface_get_ifname(GVirConfigDomainInterface *interface)
>
> Unless I'm missing something, this should not be const (caller needs to
> free the returned string).
String getters usually do return const in the gobject world[1] as
opposed to object getters as one require allocation/de-allocation and
the other only requires incrementing/decrementing a counter. Also is
the fact that strings are readily usable so you can just make a call
to the getter and thats it but objects are not usually readily usable
in the sense that you have to pass it to to some other function to do
something with it. Note that I'm just making educated guesses here as
to why 'const' for strings makes more sense as to why this practice is
followed.
>> +{
>> + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_INTERFACE(interface), NULL);
>> +
>> + return gvir_config_object_get_attribute(GVIR_CONFIG_OBJECT(interface),
>> + "target", "device");
>
> This is "dev", not "device"
OK
>> +}
>> +
>> +GVirConfigDomainInterfaceLinkState gvir_config_domain_interface_get_link_state(GVirConfigDomainInterface *interface)
>> +{
>> + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_INTERFACE(interface),
>> + GVIR_CONFIG_DOMAIN_INTERFACE_LINK_STATE_DEFAULT);
>> +
>> + return gvir_config_object_get_attribute_genum(GVIR_CONFIG_OBJECT(interface),
>> + "link", "state",
>> + GVIR_CONFIG_TYPE_DOMAIN_INTERFACE_LINK_STATE,
>> + GVIR_CONFIG_DOMAIN_INTERFACE_LINK_STATE_DEFAULT);
>> +}
>
> Have you tested this? Is it available for reading after having been set?
> The reason I'm asking is that libvirt documentation only talk about
> modifying it.
No, I haven't really tested this part. The only test I have done is
with the Boxes patch[2].
>> diff --git a/libvirt-gconfig/libvirt-gconfig.sym b/libvirt-gconfig/libvirt-gconfig.sym
>> index 96ce58f..f91b8b0 100644
>> --- a/libvirt-gconfig/libvirt-gconfig.sym
>> +++ b/libvirt-gconfig/libvirt-gconfig.sym
>> @@ -145,6 +145,10 @@ LIBVIRT_GCONFIG_0.0.4 {
>> gvir_config_domain_interface_set_link_state;
>> gvir_config_domain_interface_set_mac;
>> gvir_config_domain_interface_set_model;
>> + gvir_config_domain_interface_get_ifname;
>> + gvir_config_domain_interface_get_link_state;
>> + gvir_config_domain_interface_get_mac;
>> + gvir_config_domain_interface_get_model;
>
> The other sections in this file put getter and setter for the same property
> next to each other, I'd prefer if we stayed consistant:
Sure.
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124
[1] http://developer.gnome.org/gtk/2.24/GtkWindow.html#gtk-window-get-icon-name
http://developer.gnome.org/gtk/2.24/GtkWindow.html#gtk-window-get-title
http://developer.gnome.org/gio/stable/GFileInfo.html#g-file-info-get-name
http://developer.gnome.org/gio/stable/GFileInfo.html#g-file-info-get-display-name
I can provide many many more examples for core gnome libs if you
like but I guess you get the point :)
[2] https://bugzilla.gnome.org/show_bug.cgi?id=670994
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list