On Sun, Feb 19, 2012 at 2:50 PM, Christophe Fergeau <cfergeau(a)redhat.com> wrote:
On Sat, Feb 18, 2012 at 07:24:03PM +0200, Zeeshan Ali (Khattak)
wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org>
>
> Mostly just a wrapper around virNodeGetInfo() and virNodeInfo struct.
Apart from the 2 small comments below, I'm wondering if we should introduce
a GVirNode class to wrap the virNode* methods. There are currently 5 to 10
such methods (forgot the exact numbers), if more are to come, this would
"bloat" the GVirConnection class. Any thoughts on that?
While I agree with you about the "bloat" part, I think the app
developer experience is more important. Since I don't know why libvirt
has put these functions under a different prefix (you couldn't figure
either IIRC?), I wonder if any app developer will be able to figure
why he is asked to create/get a different object here (especially
after he looks at underlying libvirt API). In this particular case,
he'll be asked to make 3 calls just to get the amount of RAM on the
host.
Looking at the following commented-out declarations in
libvirt-gobject/libvirt-gobject-connection.h:
GList *gvir_connection_get_node_devices(GVirConnection *conn);
GVirNodeDevice *gvir_connection_get_node_device(GVirConnection *conn,
const gchar *name);
I'm guessing Daniel intended to handle it the way my patch is doing.
> + ret = g_slice_new(GVirNodeInfo);
> + g_memmove (ret->model, info.model, sizeof (ret->model));
Why not g_strncpy here?
Cause I always forget which one is most appropriate. :) Thanks for pointing out.
> #endif /* __LIBVIRT_GOBJECT_CONNECTION_H__ */
> diff --git a/libvirt-gobject/libvirt-gobject.sym
b/libvirt-gobject/libvirt-gobject.sym
> index 7a2f65d..9e63773 100644
> --- a/libvirt-gobject/libvirt-gobject.sym
> +++ b/libvirt-gobject/libvirt-gobject.sym
> @@ -4,6 +4,7 @@ LIBVIRT_GOBJECT_0.0.4 {
> gvir_init_object_check;
>
> gvir_connection_get_type;
> + gvir_node_info_get_type;
> gvir_connection_new;
> gvir_connection_open;
> gvir_connection_open_async;
gvir_connection_get_node_info is missing here.
Yup.
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124