[libvirt] [RFC] [libvirt-gconfig] Suggestion about (maybe) re-factoring GVirtConfigDomainGraphics
Howdy! I've been trying to use libvirt-gobject and libvirt-gconfig, on virt-viewer, for accessing VMs and looking at their config, instead of using libvrit and parsing XML directly and turns out, that libvirt-gconfig is not exactly handful for a "read-only" use case (as virt-viewer's one). Let me try to explain pointing to some code as example and then I'll give my suggestions. For example, let's take a look on https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/virt-viewer.c#n46... In this function, the first thing done is to get the type of the graphic device (SPICE or VNC) and here is the first problem. There is no straightforward way for doing this on libvirt-gconfig (or is there?). Seems easier to continue getting the type using libxml and then use the specific spice/vnc functions for getting the properties. And here is the second problem, because I'll always need to have an if/else statement for getting the properties. Something like: if (g_str_equal(type, "vnc")) port = gvir_config_domain_graphics_vnc_get_port(domain); else if (g_str_equal(type, "spice")) port = gvir_config_domain_graphics_spice_get_port(domain); This kind of usage makes me think that libvirt-gconfig is missing some abstraction class, parent of GVirConfigDomainGraphics{Spice,Vnc,...) which could provide virtual functions like: gtype = gvir_config_domain_graphics_get_gtype(domain); port = gvir_config_domain_graphics_get_port(domain); Thinking a bit about it and taking a look in the GVirConfigDomainGraphics code, is possible to see a possibility for 2 new classes: GVirConfigDomainGraphicsLocal and GVirConfigDomainGraphicsRemote. Then we could have something like: GVirtConfigDomainGraphics |_ | GVirtConfigDomainGraphicsLocal | |_ | | GVirtConfigDomainGraphicsLocalDesktop | |_ | GVirtConfigDomainGraphicsLocalSdl |_ GVirtConfigDomainGraphicsRemote |_ | GVirtConfigDomainGraphicsRemoteSpice |_ | GVirtConfigDomainGraphicsRemoteVnc |_ GVirtConfigDomainGraphicsRemoteRdp I do know that Local and Remote are not exactly accurate names, but is the best that I could come up with. So, would be acceptable to introduce these two new classes and then have the specific graphics classes inheriting from those? Does this make sense for you, people? I'm more than happy in provide the code for this, but not before we discuss and set a decision about the approach. :-) I'm looking forward for some feedback. -- Fabiano Fidêncio
On Mon, Feb 29, 2016 at 11:51:26PM +0100, Fabiano Fidêncio wrote:
Howdy!
I've been trying to use libvirt-gobject and libvirt-gconfig, on virt-viewer, for accessing VMs and looking at their config, instead of using libvrit and parsing XML directly and turns out, that libvirt-gconfig is not exactly handful for a "read-only" use case (as virt-viewer's one).
Let me try to explain pointing to some code as example and then I'll give my suggestions. For example, let's take a look on https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/virt-viewer.c#n46...
In this function, the first thing done is to get the type of the graphic device (SPICE or VNC) and here is the first problem. There is no straightforward way for doing this on libvirt-gconfig (or is there?). Seems easier to continue getting the type using libxml and then use the specific spice/vnc functions for getting the properties. And here is the second problem, because I'll always need to have an if/else statement for getting the properties. Something like: if (g_str_equal(type, "vnc")) port = gvir_config_domain_graphics_vnc_get_port(domain); else if (g_str_equal(type, "spice")) port = gvir_config_domain_graphics_spice_get_port(domain);
This kind of usage makes me think that libvirt-gconfig is missing some abstraction class, parent of GVirConfigDomainGraphics{Spice,Vnc,...) which could provide virtual functions like: gtype = gvir_config_domain_graphics_get_gtype(domain); port = gvir_config_domain_graphics_get_port(domain);
Thinking a bit about it and taking a look in the GVirConfigDomainGraphics code, is possible to see a possibility for 2 new classes: GVirConfigDomainGraphicsLocal and GVirConfigDomainGraphicsRemote.
Then we could have something like: GVirtConfigDomainGraphics |_ | GVirtConfigDomainGraphicsLocal | |_ | | GVirtConfigDomainGraphicsLocalDesktop | |_ | GVirtConfigDomainGraphicsLocalSdl |_ GVirtConfigDomainGraphicsRemote |_ | GVirtConfigDomainGraphicsRemoteSpice |_ | GVirtConfigDomainGraphicsRemoteVnc |_ GVirtConfigDomainGraphicsRemoteRdp
I do know that Local and Remote are not exactly accurate names, but is the best that I could come up with.
So, would be acceptable to introduce these two new classes and then have the specific graphics classes inheriting from those? Does this make sense for you, people?
I'm more than happy in provide the code for this, but not before we discuss and set a decision about the approach. :-)
As long as you can introduce the new classes without breaking API compatability of existing classes, I think the idea is reasonable. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Hey, On Mon, Feb 29, 2016 at 11:51:26PM +0100, Fabiano Fidêncio wrote:
Howdy!
I've been trying to use libvirt-gobject and libvirt-gconfig, on virt-viewer, for accessing VMs and looking at their config, instead of using libvrit and parsing XML directly and turns out, that libvirt-gconfig is not exactly handful for a "read-only" use case (as virt-viewer's one).
Let me try to explain pointing to some code as example and then I'll give my suggestions. For example, let's take a look on https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/virt-viewer.c#n46...
In this function, the first thing done is to get the type of the graphic device (SPICE or VNC) and here is the first problem. There is no straightforward way for doing this on libvirt-gconfig (or is there?). Seems easier to continue getting the type using libxml and then use the specific spice/vnc functions for getting the properties. And here is the second problem, because I'll always need to have an if/else statement for getting the properties. Something like: if (g_str_equal(type, "vnc")) port = gvir_config_domain_graphics_vnc_get_port(domain); else if (g_str_equal(type, "spice")) port = gvir_config_domain_graphics_spice_get_port(domain);
For what it's worth, you could cheat by adding some gobject properties on both classes, and do g_object_get(G_OBJECT(graphics), "port", &port, NULL); and expect 2 properties with the same name to be defined in the 2 classes. I agree this is not a great suggestion though ;)
This kind of usage makes me think that libvirt-gconfig is missing some abstraction class, parent of GVirConfigDomainGraphics{Spice,Vnc,...) which could provide virtual functions like: gtype = gvir_config_domain_graphics_get_gtype(domain);
I'd prefer to avoid this one, it's not much different from what you can already achieve with g_type_name (G_OBJECT_TYPE (graphics)), or even better, with GVIR_CONFIG_IS_DOMAIN_GRAPHICS_SPICE(graphics) / GVIR_CONFIG_IS_DOMAIN_GRAPHICS_VNC(graphics) As this is a bit low-level, we could have some helpers to avoid doing these for common cases, but in this case, I'd expect something higher level than _get_gtype(). You mention "virtual functions", if by that you mean function pointers in the parent class which are overriden by the vnc/spice child classes, I'd try to avoid them if possible, and move the (hopefully) duplicate logic in the child classes toward the parent class, and keep API/ABI by just having int gvir_config_domain_graphics_spice_get_port(GVirConfigDomainGraphicsSpice *graphics) { return gvir_config_domain_graphics_get_port(GVIR_CONFIG_DOMAIN_GRAPHICS_REMOTE(graphics)); } Christophe
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Fabiano Fidêncio