GNOME Bugzilla – Bug 681747
Add oVirt support
Last modified: 2016-03-31 14:00:43 UTC
This patch series adds basic support to access oVirt VMs from boxes. librest from git is needed for now, as well as libgovirt from http://people.freedesktop.org/~teuf/govirt/libgovirt-0.0.1.tar.xz One pending issue with these patches is proper conditional compilation of the govirt stuff...
Created attachment 220986 [details] [review] ovirt: add rest-0.7 vapi We need a vapi file binding the librest API from git, ship our own for now until librest gets a new release, and vala ships an updated binding.
Created attachment 220987 [details] [review] ovirt: add govirt vapi file
Created attachment 220988 [details] [review] ovirt: allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 220989 [details] [review] spice: allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 220990 [details] [review] spice: add SpiceDisplay::ca_cert property When a TLS SPICE connection is used, a CA certificate must be known to the SPICE connection in order to validate the certificate presented by the server we are connecting to.
Created attachment 220991 [details] [review] ovirt: implement OvirtMachine This is used to control oVirt VMs.
Created attachment 220992 [details] [review] ovirt: fetch and use CA certificate from oVirt server The various hosts that can be connected to through the oVirt REST API will all provide a certificate which is signed by a CA certificate available on the oVirt REST server under https://hostname/ca.crt Use the API from libgovirt to fetch this certificate and use it when creating the SPICE connection.
Created attachment 220993 [details] [review] ovirt: create OvirtMachine objects for oVirt sources This commit updates the oVirt source to create oVirt machines from the list of VMs available on the remote oVirt host.
Created attachment 220994 [details] [review] ovirt: add a way to set the SPICE host subject In some circumstances, the certificate the SPICE host we are connecting to will no be valid for the IP we are connecting to. When this happens, a 'host subject' has to be provided to spice-gtk so that it can authenticate the host it's connecting to. This has been implemented very recently in oVirt 3.1 REST API, see https://bugzilla.redhat.com/show_bug.cgi?id=807384 but requires some non-trivial work in libgovirt. In the mean time, add a BOXES_SPICE_HOST_SUBJECT environment variable for the cases when it's required, but it's really a hack and should be solved by using this new REST API. The host subject looks like O=organization,CN=hostname, where organization is an arbitrary organization name, and hostname is the hostname/IP the certificate was generated for. If the host address hasn't changed, its IP should work. It can happen that the host address isn't the SPICE address we are connecting to since oVirt can have different management/display addresses.
Created attachment 220995 [details] [review] ovirt: move ovirt/boxes state mapping to a separate method
Created attachment 220996 [details] [review] ovirt: attempt to start stopped VMs before connecting
Created attachment 220997 [details] [review] ovirt: add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 220998 [details] [review] machine: fix some methods visibility Some methods from the Machine class were public while they are not really useful in the public interface. Mark these as protected or private.
Created attachment 220999 [details] [review] ovirt: load saved screenshot in OvirtMachine() oVirt machines that have already been connected to once will show a non-default thumbnail.
Created attachment 221000 [details] [review] ovirt: implement OvirtMachine::take_screenshot This makes it possible to reuse the machinery which takes regular box screenshot. This implementation of take_screenshot is arguably not really useful since screenshots will only be taken while the box is maximized/fullscreen, and the disconnect_display () logic takes care of taking a screenshot just before going back to the collection view. Taking regular screenshots can have its use in case of an abnormal gnome-boxes termination.
Created attachment 221001 [details] [review] CollectionView: mark internal method as private The only user of this method is in CollectionView.
ftp://ftp.gnome.org/pub/gnome/sources/rest/0.7/rest-0.7.90.tar.xz has been released now.
Review of attachment 220986 [details] [review]: - now that rest-0.7 got released, we can depend on it - when adding a binding copy, you need to also dist it by modifying EXTRA_DIST in vapi/upstream/Makefile.am
Review of attachment 220987 [details] [review]: - why govirt-1.0.vapi binding is not generated and shipped in upstream instead? Boxes doesn't need it, it can be useful to other projects too. - you need to dist vapi bindings
Review of attachment 220988 [details] [review]: looks good ::: src/app.vala @@ +282,3 @@ + uri.path = GLib.Path.build_filename (uri.path, "api"); + + var proxy = new Ovirt.Proxy (uri.save()); nitpick, missing space
Review of attachment 220989 [details] [review]: looks good ::: src/spice-display.vala @@ +52,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, + int tls_port = 0) { it would be nice to add requires (port != 0 || tls_port != 0)
Review of attachment 220990 [details] [review]: ::: src/spice-display.vala @@ +6,3 @@ public override string protocol { get { return "SPICE"; } } public override string uri { owned get { return session.uri; } } + public string host_cert { owned get { return session.ca_file; } set { session.ca_file = value; } } "ca_file" sounds a better name than "host_cert". Why did you prefer to have a different name? The client certificate isn't the host/server certificate, but the certificate authority that can be used to verify it, correct?
Review of attachment 220991 [details] [review]: - you can remove copied commented code - empty construct { } isn't necessary - load_screenshot() at the end of ctor should help to load last saved display screenshot ::: src/ovirt-machine.vala @@ +43,3 @@ + + try { + display = create_display_connection(); style nitpick, missing space
Review of attachment 220992 [details] [review]: looks good
Review of attachment 220993 [details] [review]: looks good I suppose there is no notification yet for when VM are added or removed. But I am not sure why you need the vm.{get,set}_data<OvirtMachine> ("machine"); here. Can you explain?
Review of attachment 220994 [details] [review]: It would make more sense to be more generic and less intrusive: only modify spice-display.vala for this workaround.
Review of attachment 220995 [details] [review]: Should be squashed with "ovirt: implement OvirtMachine"
Review of attachment 220996 [details] [review]: ::: src/ovirt-machine.vala @@ +29,3 @@ + if (state == MachineState.STOPPED) + try { + vm.start (proxy); is this call synchronous? I suppose it is or there would a race. If govirt doesn't have async support, you should wrap that with run_in_thread() or similar
Review of attachment 220997 [details] [review]: What about moving that dialog code to util-app.vala or as a static method of ovirt-machine.vala? It would be nice to factor out the UI part, so that the dialog could be reused eventually for other brokers, eventually.
Review of attachment 220998 [details] [review]: once rebased, I am all for limiting the visibility of methods.
Review of attachment 220999 [details] [review]: already commented on that earlier, I guess it could be part of initial ovirt-machine.vala patch
Review of attachment 221000 [details] [review]: if we decide to use this approach, we should do it for all sources kind: remote, libvirt and ovirt for consistency, as a seperate bug.
Review of attachment 221001 [details] [review]: ack
Not answering in splinter as I don't get proper quoting there. (In reply to comment #18) > Review of attachment 220986 [details] [review]: > > - now that rest-0.7 got released, we can depend on it How do you mean? In configure.ac? Or to get the vapi file? The configure.ac dependency will come indirectly from the govirt check which needs librest, but since in the end Boxes directly call into librest, I'll add a check for it as well. As for the vapi file, I don't think librest ships a proper one, vala ships one.
Not answering in splinter as I don't get proper quoting there. (In reply to comment #19) > Review of attachment 220987 [details] [review]: > > - why govirt-1.0.vapi binding is not generated and shipped in upstream instead? > Boxes doesn't need it, it can be useful to other projects too. libgovirt ships a working .gir file which can be directly converted to a .vapi file, I don't see the point of shipping a generated file in libgovirt, vala should just use the gir file.
Review of attachment 220989 [details] [review]: ::: src/spice-display.vala @@ +52,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, + int tls_port = 0) { How should this be aligned ? ;) I don't want to change this patch several times just to get the alignement right.
Not answering in splinter as I don't get proper quoting there. (In reply to comment #22) > Review of attachment 220990 [details] [review]: > > ::: src/spice-display.vala > @@ +6,3 @@ > public override string protocol { get { return "SPICE"; } } > public override string uri { owned get { return session.uri; } } > + public string host_cert { owned get { return session.ca_file; } set { > session.ca_file = value; } } > > "ca_file" sounds a better name than "host_cert". Why did you prefer to have a > different name? > I probably did this before I got a correct understanding of all this certificate stuff. I might prefer ca_cert over ca_file though, but I agree that either of these is better than host_cert > The client certificate isn't the host/server certificate, but the certificate > authority that can be used to verify it, correct? Yes, correct, but took me a while to get this ;)
(In reply to comment #35) > Not answering in splinter as I don't get proper quoting there. > > (In reply to comment #19) > > Review of attachment 220987 [details] [review] [details]: > > > > - why govirt-1.0.vapi binding is not generated and shipped in upstream instead? > > Boxes doesn't need it, it can be useful to other projects too. > > libgovirt ships a working .gir file which can be directly converted to a .vapi > file, I don't see the point of shipping a generated file in libgovirt, But insn't .gir also generated? > vala > should just use the gir file. Depends on the API so if you API is straight forward enough, it should just work. If it does, we don't need to have a vapi file at all (even in Boxes).
Review of attachment 221000 [details] [review]: I can move that code to the base class indeed, libvirt overrides take_screenshot though to implement a better libvirt-specific behaviour.
(splinter + !quoting again) (In reply to comment #25) > Review of attachment 220993 [details] [review]: > I suppose there is no notification yet for when VM are added or removed. Yes, there is no such thing in libgovirt yet. Worse than that, I don't think oVirt REST API provides such a thing, and when I ask the developers didn't really seem to see a point in this kind of notifications... In other words, we might have to do some polling to watch VM states at first :( > But I > am not sure why you need the vm.{get,set}_data<OvirtMachine> ("machine"); > here. Can you explain? Just copy&paste from the libvirt code, it may indeed be totally useless, I'll check that.
(In reply to comment #38) > (In reply to comment #35) > > Not answering in splinter as I don't get proper quoting there. > > > > (In reply to comment #19) > > > Review of attachment 220987 [details] [review] [details] [details]: > > > > > > - why govirt-1.0.vapi binding is not generated and shipped in upstream instead? > > > Boxes doesn't need it, it can be useful to other projects too. > > > > libgovirt ships a working .gir file which can be directly converted to a .vapi > > file, I don't see the point of shipping a generated file in libgovirt, > > But insn't .gir also generated? It's not trivial to generate if you don't have the source tree as far as I know (I may be confusing things with typelibs), and the difference between the .gir and the .vapi is that the .gir is supposed to be the way of getting bindings for any language with .gir support. I don't see the point of adding binding specific code upstream if the .gir is correctly generated.(In reply to comment #38) > Depends on the API so if you API is straight forward enough, it should just > work. If it does, we don't need to have a vapi file at all (even in Boxes). this .vapi file is just a vapigen run against the .gir file (with a few dependant packages specified on the cmdline), no tweaking of the resulting file needed. If there is more needed to make the .gir file more useful, just let me know.
(In reply to comment #35) > Not answering in splinter as I don't get proper quoting there. > > (In reply to comment #19) > > Review of attachment 220987 [details] [review] [details]: > > > > - why govirt-1.0.vapi binding is not generated and shipped in upstream instead? > > Boxes doesn't need it, it can be useful to other projects too. > > libgovirt ships a working .gir file which can be directly converted to a .vapi > file, I don't see the point of shipping a generated file in libgovirt, vala > should just use the gir file. So you don't need to generate a vapi file if you don't have any vala annotations (that do not exist in gir yet). You can use the gir directly.
Review of attachment 220989 [details] [review]: ::: src/spice-display.vala @@ +52,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, + int tls_port = 0) { if you asked with a smile, you probably know my rationale on this. Anyway, that was just a suggestion, feel free to ignore
Review of attachment 220989 [details] [review]: ::: src/spice-display.vala @@ +52,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, + int tls_port = 0) { I'll add it, I know you discussed this recently with Zeeshan, I don't know if you came to an agreement, that's why I'm asking. Personally I'd indent it by a few chars (I did 8 or 16 iirc last time) from the method declaration, but Zeeshan wanted it at the end of the line. I'll do whatever the consensus is.
Review of attachment 220994 [details] [review]: I'm considering dropping this patch since it's very suboptimal anyway, if you want different host subjects, you have to restart boxes to change it every time. So I'm not sure if this workaround is worth having for now.
Review of attachment 220996 [details] [review]: ::: src/ovirt-machine.vala @@ +29,3 @@ + if (state == MachineState.STOPPED) + try { + vm.start (proxy); Ah good catch, connect_display was sync when I started this series, I'll switch to the async start variant.
Created attachment 221374 [details] [review] ovirt: conditionally use libgovirt during build The compilation of oVirt specific stuff will depend on a Vala define, this unfortunately doesn't work nicely when shipping pregenerated C files in tarballs.
Created attachment 221375 [details] [review] ovirt: add rest-0.7 vapi We need a vapi file binding the librest 0.7.90 API, ship our own for now until vala ships an updated binding.
Created attachment 221376 [details] [review] ovirt: allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 221378 [details] [review] spice: allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 221379 [details] [review] spice: add SpiceDisplay::ca_file property When a TLS SPICE connection is used, a CA certificate must be known to the SPICE connection in order to validate the certificate presented by the server we are connecting to.
Created attachment 221380 [details] [review] ovirt: implement OvirtMachine This is used to control oVirt VMs.
Created attachment 221381 [details] [review] ovirt: fetch and use CA certificate from oVirt server The various hosts that can be connected to through the oVirt REST API will all provide a certificate which is signed by a CA certificate available on the oVirt REST server under https://hostname/ca.crt Use the API from libgovirt to fetch this certificate and use it when creating the SPICE connection.
Created attachment 221382 [details] [review] ovirt: create OvirtMachine objects for oVirt sources This commit updates the oVirt source to create oVirt machines from the list of VMs available on the remote oVirt host.
Created attachment 221383 [details] [review] ovirt: add a way to set the SPICE host subject In some circumstances, the certificate the SPICE host we are connecting to will no be valid for the IP we are connecting to. When this happens, a 'host subject' has to be provided to spice-gtk so that it can authenticate the host it's connecting to. This has been implemented very recently in oVirt 3.1 REST API, see https://bugzilla.redhat.com/show_bug.cgi?id=807384 but requires some non-trivial work in libgovirt. In the mean time, add a BOXES_SPICE_HOST_SUBJECT environment variable for the cases when it's required, but it's really a hack and should be solved by using this new REST API. The host subject looks like O=organization,CN=hostname, where organization is an arbitrary organization name, and hostname is the hostname/IP the certificate was generated for. If the host address hasn't changed, its IP should work. It can happen that the host address isn't the SPICE address we are connecting to since oVirt can have different management/display addresses.
Created attachment 221384 [details] [review] ovirt: attempt to start stopped VMs before connecting
Created attachment 221385 [details] [review] ovirt: add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 221386 [details] [review] machine: fix some methods visibility Some methods from the Machine class were public while they are not really useful in the public interface. Mark these as protected or private.
Created attachment 221388 [details] [review] ovirt: implement OvirtMachine::take_screenshot This makes it possible to reuse the machinery which takes regular box screenshot. This implementation of take_screenshot is arguably not really useful since screenshots will only be taken while the box is maximized/fullscreen, and the disconnect_display () logic takes care of taking a screenshot just before going back to the collection view. Taking regular screenshots can have its use in case of an abnormal gnome-boxes termination.
Created attachment 221389 [details] [review] CollectionView: mark internal method as private The only user of this method is in CollectionView.
Forgot to mention that libgovirt from git is needed http://cgit.freedesktop.org/~teuf/govirt/ , found a small issue with async VM methods while addressing the "vm.start is sync" comment.
Any comments on these patches ? :)
(In reply to comment #62) > Any comments on these patches ? :) Marc-Andre looked at them and he said he is good with these patches so I guess its an ACK then. Having said that, we both know that we completely disagree about vapi file distribution. I do not intend to start another discussion about that and will simply accept your decision to keep vapi in boxes if its ok with Marc-Andre (?) but please keep in mind that it will be your responsibility to keep it up2date with latest govirt.
(In reply to comment #63) > Having said that, we both know that we completely disagree about vapi file > distribution. I do not intend to start another discussion about that and will > simply accept your decision to keep vapi in boxes if its ok with Marc-Andre (?) > but please keep in mind that it will be your responsibility to keep it up2date > with latest govirt. I'm all for using the gir file directly but I didn't really figure out how to do it, help welcome.
(In reply to comment #64) > (In reply to comment #63) > > Having said that, we both know that we completely disagree about vapi file > > distribution. I do not intend to start another discussion about that and will > > simply accept your decision to keep vapi in boxes if its ok with Marc-Andre (?) > > but please keep in mind that it will be your responsibility to keep it up2date > > with latest govirt. > > I'm all for using the gir file directly but I didn't really figure out how to > do it, help welcome. I seriously doubt we can get that stuff to work reliably in next few weeks, everyone uses vapi for a reason. Now your patches can go in already (with feature freeze break permission from r-t@gnome) if you promise to maintain the vapi in Boxes for us.
(In reply to comment #65) > I seriously doubt we can get that stuff to work reliably in next few weeks, > everyone uses vapi for a reason. Even if we cannot get it to work, trying it and filing bugs would be a first step...
(In reply to comment #66) > (In reply to comment #65) > > I seriously doubt we can get that stuff to work reliably in next few weeks, > > everyone uses vapi for a reason. > > Even if we cannot get it to work, trying it and filing bugs would be a first > step... No disagreement there, I'm just not interested too much in that.
Created attachment 223255 [details] [review] move authentication dialog to util-app.vala
Created attachment 223256 [details] [review] Add generic broker support to App Add some code in App to let brokers register the uri schemes they know how to handle, and to automatically use the registered brokers. This is achieved through the addition of a Broker base class. However, I'm not sure it's possible to have broker registration run automatically at the application startup, so we may still need a bit of broker specific code in App.vala :(
Created attachment 223257 [details] [review] libvirt: move most libvirt code from App to a Broker This moves most of the libvirt specific code out of App to a new LibvirtBroker class, making App more generic.
Created attachment 223258 [details] [review] libvirt: Move App::add_domain to LibvirtBroker
Created attachment 223259 [details] [review] ovirt: turn ovirt-specific code into a Broker impl Rather than having ~70 ovirt-specific lines of code in the App class, it's better to move this code in an oVirt broker class as this code does not need to use private App data anyway. Make OvirtBroker inherit from Broker
Created attachment 223260 [details] [review] ovirt: remove unused code
Created attachment 223261 [details] [review] ovirt: improve conditional ovirt compilation Currently, oVirt is conditionally compiled through use of #if in vala code. However, this does not work as expected with tarballs that ship generated C files, the #if are not propagated to the generated C code. Now that all the oVirt code is self-contained in ovirt-machine.vala save for an initialization call in app.vala, we can now do better by having a file provide a stub implementation for this initialization call. Given that the conditional compilation of files is controlled by automake, this will work both for vala files, and when using tarballs with pregenerated C files.
These new patches refactor the libvirt/ovirt machine creation code to remove most of the ovirt/libvirt creation code from App.vala. The main goal of this series is to make conditional compilation of the ovirt code work both from git and tarballs, but regardless of that, I think this refactoring is worthwhile.
can you clean up, rebase/squash the series, so all the patches apply and can be reviewed one by one? thanks
Created attachment 223297 [details] [review] Add generic broker support to App Add some code in App to let brokers register the uri schemes they know how to handle, and to automatically use the registered brokers. This is achieved through the addition of a Broker base class. However, I'm not sure it's possible to have broker registration run automatically at the application startup, so we may still need a bit of broker specific code in App.vala :(
Created attachment 223298 [details] [review] libvirt: move most libvirt code from App to a Broker This moves most of the libvirt specific code out of App to a new LibvirtBroker class, making App more generic.
Created attachment 223299 [details] [review] libvirt: Move App::add_domain to LibvirtBroker
Created attachment 223300 [details] [review] ovirt: conditionally use libgovirt during build The compilation of oVirt specific stuff will depend on a Vala define, this unfortunately doesn't work nicely when shipping pregenerated C files in tarballs.
Created attachment 223301 [details] [review] ovirt: add rest-0.7 vapi We need a vapi file binding the librest 0.7.90 API, ship our own for now until vala ships an updated binding.
Created attachment 223302 [details] [review] ovirt: add oVirt broker For now, the broker isn't doing much, but more will be added in later commits
Created attachment 223303 [details] [review] ovirt: allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 223304 [details] [review] spice: allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 223305 [details] [review] spice: add SpiceDisplay::ca_file property When a TLS SPICE connection is used, a CA certificate must be known to the SPICE connection in order to validate the certificate presented by the server we are connecting to.
Created attachment 223306 [details] [review] ovirt: implement OvirtMachine This is used to control oVirt VMs.
Created attachment 223307 [details] [review] ovirt: fetch and use CA certificate from oVirt server The various hosts that can be connected to through the oVirt REST API will all provide a certificate which is signed by a CA certificate available on the oVirt REST server under https://hostname/ca.crt Use the API from libgovirt to fetch this certificate and use it when creating the SPICE connection.
Created attachment 223308 [details] [review] ovirt: create OvirtMachine objects for oVirt sources This commit updates the oVirt source to create oVirt machines from the list of VMs available on the remote oVirt host.
Created attachment 223309 [details] [review] ovirt: add a way to set the SPICE host subject In some circumstances, the certificate the SPICE host we are connecting to will no be valid for the IP we are connecting to. When this happens, a 'host subject' has to be provided to spice-gtk so that it can authenticate the host it's connecting to. This has been implemented very recently in oVirt 3.1 REST API, see https://bugzilla.redhat.com/show_bug.cgi?id=807384 but requires some non-trivial work in libgovirt. In the mean time, add a BOXES_SPICE_HOST_SUBJECT environment variable for the cases when it's required, but it's really a hack and should be solved by using this new REST API. The host subject looks like O=organization,CN=hostname, where organization is an arbitrary organization name, and hostname is the hostname/IP the certificate was generated for. If the host address hasn't changed, its IP should work. It can happen that the host address isn't the SPICE address we are connecting to since oVirt can have different management/display addresses.
Created attachment 223310 [details] [review] ovirt: attempt to start stopped VMs before connecting
Created attachment 223311 [details] [review] ovirt: add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 223312 [details] [review] machine: fix some methods visibility Some methods from the Machine class were public while they are not really useful in the public interface. Mark these as protected or private.
Created attachment 223313 [details] [review] ovirt: implement OvirtMachine::take_screenshot This makes it possible to reuse the machinery which takes regular box screenshot. This implementation of take_screenshot is arguably not really useful since screenshots will only be taken while the box is maximized/fullscreen, and the disconnect_display () logic takes care of taking a screenshot just before going back to the collection view. Taking regular screenshots can have its use in case of an abnormal gnome-boxes termination.
Created attachment 223314 [details] [review] CollectionView: mark internal method as private The only user of this method is in CollectionView.
(In reply to comment #76) > can you clean up, rebase/squash the series, so all the patches apply and can be > reviewed one by one? thanks Done, please note I haven't tried to run this yet, so it may not be working. My goal at this point is to get feedback on this Broker approach.
Created attachment 227252 [details] [review] Add generic broker support to App Add some code in App to let brokers register the uri schemes they know how to handle, and to automatically use the registered brokers. This is achieved through the addition of a Broker base class. However, I'm not sure it's possible to have broker registration run automatically at the application startup, so we may still need a bit of broker specific code in App.vala :(
Created attachment 227253 [details] [review] libvirt: move most libvirt code from App to a Broker This moves most of the libvirt specific code out of App to a new LibvirtBroker class, making App more generic.
Created attachment 227254 [details] [review] libvirt: Move App::add_domain to LibvirtBroker
Created attachment 227255 [details] [review] ovirt: conditionally use libgovirt during build
Created attachment 227256 [details] [review] ovirt: add oVirt broker For now, the broker isn't doing much, but more will be added in later commits
Created attachment 227257 [details] [review] ovirt: allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 227258 [details] [review] spice: allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 227259 [details] [review] spice: add SpiceDisplay::ca_file property When a TLS SPICE connection is used, a CA certificate must be known to the SPICE connection in order to validate the certificate presented by the server we are connecting to.
Created attachment 227260 [details] [review] ovirt: implement OvirtMachine This is used to control oVirt VMs.
Created attachment 227261 [details] [review] ovirt: fetch and use CA certificate from oVirt server The various hosts that can be connected to through the oVirt REST API will all provide a certificate which is signed by a CA certificate available on the oVirt REST server under https://hostname/ca.crt Use the API from libgovirt to fetch this certificate and use it when creating the SPICE connection.
Created attachment 227262 [details] [review] ovirt: create OvirtMachine objects for oVirt sources This commit updates the oVirt source to create oVirt machines from the list of VMs available on the remote oVirt host.
Created attachment 227263 [details] [review] ovirt: add a way to set the SPICE host subject In some circumstances, the certificate the SPICE host we are connecting to will no be valid for the IP we are connecting to. When this happens, a 'host subject' has to be provided to spice-gtk so that it can authenticate the host it's connecting to. This has been implemented very recently in oVirt 3.1 REST API, see https://bugzilla.redhat.com/show_bug.cgi?id=807384 but requires some non-trivial work in libgovirt. In the mean time, add a BOXES_SPICE_HOST_SUBJECT environment variable for the cases when it's required, but it's really a hack and should be solved by using this new REST API. The host subject looks like O=organization,CN=hostname, where organization is an arbitrary organization name, and hostname is the hostname/IP the certificate was generated for. If the host address hasn't changed, its IP should work. It can happen that the host address isn't the SPICE address we are connecting to since oVirt can have different management/display addresses.
Created attachment 227264 [details] [review] ovirt: attempt to start stopped VMs before connecting
Created attachment 227265 [details] [review] ovirt: add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 227266 [details] [review] machine: fix some methods visibility Some methods from the Machine class were public while they are not really useful in the public interface. Mark these as protected or private.
Created attachment 227267 [details] [review] ovirt: implement OvirtMachine::take_screenshot This makes it possible to reuse the machinery which takes regular box screenshot. This implementation of take_screenshot is arguably not really useful since screenshots will only be taken while the box is maximized/fullscreen, and the disconnect_display () logic takes care of taking a screenshot just before going back to the collection view. Taking regular screenshots can have its use in case of an abnormal gnome-boxes termination.
Created attachment 227268 [details] [review] CollectionView: mark internal method as private The only user of this method is in CollectionView.
All these patches are available in this branch http://cgit.freedesktop.org/~teuf/gnome-boxes/log/?h=ovirt
.. and it needs http://cgit.freedesktop.org/~teuf/govirt/ (of which I'll make a release shortly, only one small remote-viewer related issue to solve).
Did this make any progress, since ?
Answering in this bug to Zeeshan's comment in https://bugzilla.gnome.org/show_bug.cgi?id=666185#c7 > I was under the impression (based on what you said in last comment and what > elmarco told me) that all your patches are now awaiting for you to make the > ovirt release. The latest version of the patches in this bug are still unreviewed, please not https://bugzilla.gnome.org/show_bug.cgi?id=681747#c95 as well. What I meant in comment in comment #114 is that a release of libgovirt is needed in addition to these patches being reviewed/ack'ed. I've already explained that several times. While the libgovirt release would block committing these patches, I don't think this should block reviewing them, especially as I'm not sure what we want/do not want from this broker stuff.
Review of attachment 227252 [details] [review]: Looks ok.
Review of attachment 227253 [details] [review]: Overall I like the approach you are taking here. ::: src/app.vala @@ +63,3 @@ public CollectionSource default_source { get { return sources.get ("QEMU Session"); } } + redundant newline @@ +161,3 @@ }); + + LibvirtBroker.init (); For consistency, better use the same pattern as used by other singletons and also it IMO make things more direct and readable: var broker = LibvirtBroker.get_default(); brokers.insert (uri_scheme, broker); And then there wont be need for App.register_broker(). @@ +251,1 @@ if (broker != null) { is this check needed anymore? If we want to be safe, it should be return_if_fail check. ::: src/libvirt-machine.vala @@ +3,3 @@ using Gtk; +private class Boxes.LibvirtBroker : Boxes.Broker { I think this class deserves its own file. LibvirtMachine is already a bit bloated. ::: src/vm-creator.vala @@ +11,3 @@ public InstallerMedia? install_media { get; private set; } + private Connection? connection { owned get { return App.app.default_connection; } } why owned?
Review of attachment 223301 [details] [review]: We just got rid of vapis we were keeping and were getting rotten so I'd really want to not include whole vapis as workaround unless really necessary. Could the requirement from 0.7.90 be somehow worked-around in the code using the vapi shipped in vala 0.17.3?
Review of attachment 227253 [details] [review]: ::: src/libvirt-machine.vala @@ +17,3 @@ + } + + public static GVir.Connection get_connection (string name) { Although I understand that you are keeping this static to make it easier to call this method but: * this is inconsistent with how App's methods work. With this logic, App's methods should also be static and we should have App.x rather than App.app.x everywhere in the code. * I don't think this class should be accessed directly but rather through App: App.app.default_broker.get_connection (name).
Review of attachment 227254 [details] [review]: Looks good otherwise. ::: src/libvirt-machine.vala @@ +21,3 @@ } + public static LibvirtMachine add_domain (CollectionSource source, GVir.Connection connection, GVir.Domain domain) Same concern about 'static' here.
Review of attachment 227255 [details] [review]: Looks good otherwise. ::: vapi/govirt-1.0.vapi @@ +1,1 @@ +/* govirt-1.0.vapi.vapi generated by vapigen, do not modify. */ You already know I strongly disagree on having this VAPI here and I know your opinion and I don't see us changing each others opinions so we are in a deadlock. :( I could be convinced to keep this vapi here as workaround though if you are going to push it to vala. ::: vapi/rest-0.7.vapi @@ +1,1 @@ +/* rest-0.7.vapi generated by vapigen, do not modify. */ Wasn't this included in a separate patch already?
Review of attachment 227256 [details] [review]: Some of the comments I made of LibvirtBroker patch(es) apply here as well. Looks good otherwise. ::: configure.ac @@ +55,3 @@ GUDEV_MIN_VERSION=165 OSINFO_MIN_VERSION=0.2.1 +REST_MIN_VERSION=0.7.90 shouldn't these changes be in the previous patch? ::: src/ovirt-machine.vala @@ +1,1 @@ +// This file is part of GNOME Boxes. License: LGPLv2+ The natural name for this file should be ovirt-broker.vala
Review of attachment 227257 [details] [review]: Looks good apart from the nitpick. ::: src/wizard.vala @@ +149,3 @@ prepare_for_location (this.wizard_source.uri, true); + if (source != null && ((source.source_type == "libvirt") || (source.source_type == "ovirt"))) { Don't see any readability benefit from brackets around last two conditions (the inner ones) and its inconsistent with the first condition in the same 'if' here.
Review of attachment 227258 [details] [review]: Apart from nitpicks, looks good. ::: src/spice-display.vala @@ +71,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, int tls_port = 0) + requires (port != 0 || tls_port != 0) + { will spare you nitpicking about placement of '{' here but don't hate me if i "correct" this at some point. :) @@ +76,2 @@ session.host = host; + if (port != 0) { IMHO {} here only makes the code look slightly uglier w/o any benefit to readability, not to mention its inconsistent with coding-style.
Review of attachment 227259 [details] [review]: Small nitpick on commit log: '::' is generally used for signals and ':' for props and that seems more appropriate in C (where there is no operator for prop access). In Vala '.' seems more appropriate. Yes, I'll stop now. :)
Review of attachment 227260 [details] [review]: That simple in the end? :) Cool. ::: src/ovirt-machine.vala @@ +65,3 @@ + + if (state != MachineState.RUNNING) { + warning ("oVirt VM '%s' is not RUNNING", vm.name); Shouldn't this message be shown in the UI? @@ +123,3 @@ + switch (vm.display.type) { + case Ovirt.VmDisplayType.SPICE: + return new SpiceDisplay (config, vm.display.address, (int)vm.display.port, oh ok, i can't help with nitpicks after all. :( Each argument on separate line if this doesn't fit 120 chars.
Review of attachment 227261 [details] [review]: Looks good. If the SPICE connection can not work without fetching the certificate, I don't think this patch should be separate from the one adding OvirtMachine.
Review of attachment 227262 [details] [review]: Same comment about this one. Don't see why this should be separate. ACK anyways.
Review of attachment 227263 [details] [review]: How widespread/frequent is this issue? If its rare, I suggest we wait for the non-trivial solution through govirt. This "hack" will require user to set environment variables and we really shouldn't be asking users to have to do that unless for a compelling reason. BTW, small typo in commit log: " will no be valid" -> "will not be valid".
Review of attachment 227264 [details] [review]: Looking good otherwise. IMHO this one also doesn't need to be a separate patch especially since acting on my review comments would mean you having to add a user visible string in a previous patch and then replacing it in this one. ::: src/ovirt-machine.vala @@ +83,3 @@ + this.update_state (); + } catch (GLib.Error error) { + warning ("couldn't start oVirt VM '%s': %s", vm.name, error.message); As commented in a previous patch's review, I think we want to give some indication of failure to user in the UI.
Review of attachment 227265 [details] [review]: ::: src/util-app.vala @@ +413,3 @@ + var dialog = new Gtk.Dialog.with_buttons (null, parent, + Gtk.DialogFlags.MODAL | Gtk.DialogFlags.DESTROY_WITH_PARENT, + ok_label, Gtk.ResponseType.ACCEPT); I think (by design) we want less dialogs (especially modal) in Boxes and more notification. For this particular case, we even have a mockup: https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-signin.png . So you want to use App.app.notificationbar here. Then again, its not clear from the code for me when exactly this dialog is shown. Is it when user clicks on an VM? If so this should be dealt with the same way as we deal with SPICE password input right now. i-e at UIState.CREDS state/stage. If not, ignore this paragraph.
Review of attachment 227266 [details] [review]: ACK
Review of attachment 227267 [details] [review]: Good otherwise. ::: src/machine.vala @@ +210,3 @@ + // just before going back to the collection view. Taking regular + // screenshots can have its use in case of an abnormal gnome-boxes + // termination '.' at end of sentence. Wouldn't complain about it otherwise but I got confused reading this since there is still text after this in the next line.. @@ +212,3 @@ + // termination + // screenshot thumbnails still need to be enabled with + // Machine::set_screenshot_enable I don't think this needs to be clarified as name of method is clear enough (that it takes *a* screenshot) but if you disagree, you want to clarify that you mean *periodic capture* of screenshot needs to be enabled.
Review of attachment 227268 [details] [review]: ACK
Review of attachment 227255 [details] [review]: ::: vapi/govirt-1.0.vapi @@ +1,1 @@ +/* govirt-1.0.vapi.vapi generated by vapigen, do not modify. */ I have to take back what I said in my last sentence here. Since now we can require latest 3.7 stuff: https://bugzilla.gnome.org/show_bug.cgi?id=686936#c5 I don't see any point in keeping these vapis here. Its up to you if you want to provide the govirt vapi from govirt or from vala package but NACK from my side on vapi part of this patch.
Review of attachment 223301 [details] [review]: The vala 0.18.1 on my fedora has new enough vapis, so I'm fine with dropping it. The one from 0.17.3 is too old.
Review of attachment 227255 [details] [review]: ::: vapi/govirt-1.0.vapi @@ +1,1 @@ +/* govirt-1.0.vapi.vapi generated by vapigen, do not modify. */ I'm not overly worried either way, its nice to just get something in that works. But the long term goal has to be to get this into an upstream module so that we can then remove the copy in boxes.
Review of attachment 223301 [details] [review]: *sight* splinter and big patch series... This one is part of an older version of the series but was not obsoleted. And I obviously can't mark it as obsolete from here
Review of attachment 227253 [details] [review]: ::: src/app.vala @@ +161,3 @@ }); + + LibvirtBroker.init (); Changed (I think I expected vala to support static constructors...) @@ +251,1 @@ if (broker != null) { end result is basically the same as the else block is a warning(). I'd rather keep the way it is now as it allows to have a nice warning on failures. ::: src/libvirt-machine.vala @@ +3,3 @@ using Gtk; +private class Boxes.LibvirtBroker : Boxes.Broker { ok, split. ::: src/vm-creator.vala @@ +11,3 @@ public InstallerMedia? install_media { get; private set; } + private Connection? connection { owned get { return App.app.default_connection; } } preprocessing fails with something like app.vala:61.55-61.122: error: Return value transfers ownership but method return type hasn't been declared to transfer ownership public GVir.Connection default_connection { get { return LibvirtBroker.get_default ().get_connection ("QEMU Session"); } } otherwise
Review of attachment 227254 [details] [review]: ::: src/libvirt-machine.vala @@ +21,3 @@ } + public static LibvirtMachine add_domain (CollectionSource source, GVir.Connection connection, GVir.Domain domain) Changed
Review of attachment 227255 [details] [review]: ::: vapi/govirt-1.0.vapi @@ +1,1 @@ +/* govirt-1.0.vapi.vapi generated by vapigen, do not modify. */ I'd rather not use development version of the vala preprocessor for now, so I have a strong preference for having the .vapi here for now. However I'm fine with making sure it's committed to vala.git before this patch gets in. ::: vapi/rest-0.7.vapi @@ +1,1 @@ +/* rest-0.7.vapi generated by vapigen, do not modify. */ This was an older version of the patch series, sorry about that, I said this in the older patch: "The vala 0.18.1 on my fedora has new enough vapis, so I'm fine with dropping it. The one from 0.17.3 is too old."
Review of attachment 227256 [details] [review]: ::: configure.ac @@ +55,3 @@ GUDEV_MIN_VERSION=165 OSINFO_MIN_VERSION=0.2.1 +REST_MIN_VERSION=0.7.90 Probably ::: src/ovirt-machine.vala @@ +1,1 @@ +// This file is part of GNOME Boxes. License: LGPLv2+ Yep, but then I'm adding an OvirtMachine class there, that's why it was named this way, to be similar to the libvirt one. Since the latter is split, I'll rename this one. ::: vapi/Makefile.am @@ +6,3 @@ libvirt-workaround.vapi \ govirt-1.0.vapi \ + rest-0.7.vapi \ This also belongs to the previous patch (from which it should then be dropped)
Review of attachment 227257 [details] [review]: ::: src/wizard.vala @@ +149,3 @@ prepare_for_location (this.wizard_source.uri, true); + if (source != null && ((source.source_type == "libvirt") || (source.source_type == "ovirt"))) { This should probably be changed to a look up of source.source_type in App::brokers instead of hardcoding string values. I'll have to check if this can work after the week-end.
Review of attachment 227258 [details] [review]: ::: src/spice-display.vala @@ +71,3 @@ + public SpiceDisplay (DisplayConfig config, string host, int port, int tls_port = 0) + requires (port != 0 || tls_port != 0) + { Changed, this conflicted during the rebase anyway. @@ +76,2 @@ session.host = host; + if (port != 0) { The point of { } is bug prevention, not readability. Changed.
Review of attachment 227259 [details] [review]: Changed to a single ':'
Review of attachment 227263 [details] [review]: It happens when a VM migrates from one host to another for whatever reason (load, admin intervention, host down, ...). On simple setups it will be rare, on more complex deployments I guess it could happen often. The necessary support has landed in ovirt, but I haven't looked yet at how to use it in libgovirt.
Review of attachment 227260 [details] [review]: ::: src/ovirt-machine.vala @@ +65,3 @@ + + if (state != MachineState.RUNNING) { + warning ("oVirt VM '%s' is not RUNNING", vm.name); Probably, I'll have to look at that again.
Review of attachment 227263 [details] [review]: I'd like to keep this hack for a short while though. This has its use for testing and is just one line. We can drop it once libgovirt supports it (except if we want to support ovirt 3.0).
Review of attachment 227265 [details] [review]: ::: src/util-app.vala @@ +413,3 @@ + var dialog = new Gtk.Dialog.with_buttons (null, parent, + Gtk.DialogFlags.MODAL | Gtk.DialogFlags.DESTROY_WITH_PARENT, + ok_label, Gtk.ResponseType.ACCEPT); bleh, can't properly answer to each of the paragraphs. The mockup looks like a modal dialog to me fwiw. The dialog shows up after you add an ovirt URI through the wizard, which is fine, but also every time you start up boxes after that, which is not fine (what happens if you added several ovirt connections?). I'm not sure what should be done about brokers which need a password though.
Review of attachment 227262 [details] [review]: Well, smaller commits means much easier review, that's the main reason for the split.
Review of attachment 227252 [details] [review]: Just a note on the naming, as I understand it, in the UI, qemu:///session and qemu:///system would be different brokers when we talk to a user, but what this patchset does is introduce a class named 'LibvirtBroker' which will handle all libvirt URIs (along with a CollectionSource object when needed). I can rename this to BrokerFactory or another appropriate name before this causes too much confusion.
Review of attachment 227263 [details] [review]: ACK with that that change. ::: src/spice-display.vala @@ +82,3 @@ session.tls_port = tls_port.to_string (); } + session.cert_subject = GLib.Environment.get_variable ("BOXES_SPICE_HOST_SUBJECT"); A short FIXME comment here would be nice so we don't forget to remove this when we can.
Review of attachment 227253 [details] [review]: OK, ACK with the changes you agreed on.
Review of attachment 227255 [details] [review]: ACK with rest-0.7.vapi dropped. ::: vapi/govirt-1.0.vapi @@ +1,1 @@ +/* govirt-1.0.vapi.vapi generated by vapigen, do not modify. */ Since vala follows gnome release cycles, I don't see how bumping vala requirement is different from bumping requirements on latest versions of other projects. Unstable release cycles is a good opportunity to avoid and remove hacks/workarounds IMO. Since you feel very strongly about this and you are agreeing on having this as temporary measure, I guess I can agree reluctantly here.
Review of attachment 227256 [details] [review]: ACK with agreed changes done.
Review of attachment 227265 [details] [review]: ::: src/util-app.vala @@ +413,3 @@ + var dialog = new Gtk.Dialog.with_buttons (null, parent, + Gtk.DialogFlags.MODAL | Gtk.DialogFlags.DESTROY_WITH_PARENT, + ok_label, Gtk.ResponseType.ACCEPT); the mockup is a notification, like we already use. i-e it can be ignored and if user ignores it, the action requiring auth will simply be cancelled (in this case: we wont' connect to ovirt server in question). Since we don't need to get this feedback synchronously, I think we should use notificationbar for this. We can also ask designers on how to improve this afterwards.
Review of attachment 227267 [details] [review]: Well this review was most nitpick about comments so ACK with or without changes suggested.
Review of attachment 227252 [details] [review]: 'LibvirtBroker' is not introduced by this patch AFAICT. Commented on wrong patch?
Review of attachment 227252 [details] [review]: Regardless of the patch, the naming comment still stands ;) This patch introduces Boxes.Broker.
Created attachment 234833 [details] [review] build-sys: Pass --target-glib to valac This allows valac to use functions introduced in newer glib versions. For example, it prefer g_byte_array_unref (introduced in glib 2.22) to g_byte_array_free when --target-glib is higher than 2.22
Created attachment 234834 [details] [review] ovirt: conditionally use libgovirt during build
Created attachment 234835 [details] [review] Alphabetically order vala package checks
Created attachment 234836 [details] [review] ovirt: add oVirt broker For now, the broker isn't doing much, but more will be added in later commits
Created attachment 234837 [details] [review] ovirt: allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 234838 [details] [review] spice: allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 234839 [details] [review] ovirt: implement OvirtMachine This is used to control oVirt VMs.
Created attachment 234840 [details] [review] searchbar: Allow to disable key press snooping The searchbar grabs all key presses in the collection view and use them to filter the displayed VMs. However, if we need keyboard input in another widget, the searchbar will prevent it. This commit adds methods to enable/disable this keyboard snooping when needed.
Created attachment 234841 [details] [review] ovirt: add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 234842 [details] [review] notificationbar: Fix infinite loop in cancel()
Review of attachment 227252 [details] [review]: Its been quite a while that oVirt integration has been pending so I suggest we try to get your changes in ASAP and improve things later. Feature freeze for 3.8 is in two weeks.
Review of attachment 234833 [details] [review]: ACK
Review of attachment 234834 [details] [review]: ACK
Review of attachment 234835 [details] [review]: ack
Review of attachment 234836 [details] [review]: looks good otherwise. ::: src/app.vala @@ -175,1 +175,2 @@ brokers.insert ("libvirt", LibvirtBroker.get_default ()); + if (Config.HAVE_OVIRT) Hmm.. how would conditional compilation work when we don't have our own copy of govirt vapi anymore? Wouldn't that introduce some ugly looking #ifdefs ? If so, I'd rather we add a hard dep on govirt. Not something that necessarily needs to be addressed now though. ::: src/ovirt-broker.vala @@ +37,3 @@ + yield proxy.fetch_vms_async (null); + } catch (GLib.Error error) { + App.app.notificationbar.display_error (_("Connection to oVirt broker failed")); we probably want to display the error.message on debug log.
Review of attachment 234837 [details] [review]: Looks good apart from the nitpick. ::: src/app.vala @@ +187,3 @@ } + public bool has_broker_for_scheme (string scheme) { I'd name it 'has_broker_for_type' or 'has_broker_for_source_type' as for libvirt broker, 'libvirt' isn't the scheme of its URI but rather 'qemu' or 'qemu+unix'.
Review of attachment 234838 [details] [review]: ACK
Review of attachment 234839 [details] [review]: ::: configure.ac @@ +53,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.1.5 LIBXML2_MIN_VERSION=2.7.8 +SPICE_GTK_MIN_VERSION=0.15 If you could mention this bump in commit log, I won't ask you to put this in separate patch. :) ::: src/ovirt-broker.vala @@ -1,3 @@ // This file is part of GNOME Boxes. License: LGPLv2+ using Ovirt; -using Gtk; seems unrelated. I guess a minor rebase mistake? ::: src/ovirt-machine.vala @@ +7,3 @@ + private Ovirt.Proxy proxy; + + void update_state () { Explicit visibility needed here. Also I prefer public methods first and constructor above all but not a requirement. @@ +92,3 @@ + private Display create_display_connection () throws GLib.Error { + if (vm.display.address == null || vm.display.address == "") + throw new Boxes.Error.INVALID ("empty display address for %s", vm.name); you don't like capitalizing, do you? :)
Review of attachment 234836 [details] [review]: ::: src/app.vala @@ -175,1 +175,2 @@ brokers.insert ("libvirt", LibvirtBroker.get_default ()); + if (Config.HAVE_OVIRT) NM this comment, I got my answer while reviewing a previous patch that now appears later in the list.
Review of attachment 234840 [details] [review]: ::: src/searchbar.vala @@ +28,3 @@ } + public void disable_key_handler () { I'd rather use one boolean property: public bool enable_key_handler { set { if (value) // unblock else // block } }
Review of attachment 234841 [details] [review]: Looks good otherwise. ::: src/notificationbar.vala @@ +112,3 @@ + + auth_button.clicked.connect ( () => { + if (auth_func != null) auth_func (username_entry.get_text (), password_entry.get_text ()); I'd put the auth_func call on new line. ::: src/util-app.vala @@ +464,3 @@ } + public bool show_authentication_dialog (string message, string ok_label, Gtk.Window parent, out string username, out string password) { Is this used or just got forgotten during rework?
Review of attachment 234842 [details] [review]: ack
Review of attachment 234840 [details] [review]: ::: src/searchbar.vala @@ +28,3 @@ } + public void disable_key_handler () { Imo this looks uglier, especially searchbar.enable_key_handler = false;, and it's not really appropriate for a property but I've changed it.
Review of attachment 234834 [details] [review]: ::: src/Makefile.am @@ +129,3 @@ +if HAVE_OVIRT +AM_VALAFLAGS += --pkg govirt-1.0 --pkg rest-0.7 Seems like this AM_VALAFLAGS line should not be in the if HAVE_OVIRT block since .vala -> .c generation runs on all files, including the one that are only conditionally compiled, and to convert these files you need to have the needed packages in AM_VALAFLAGS.
I would suggest you already merge commits until and including 'Alphabetically order vala package checks' (attachment#234835 [details] here and commit 85404a0 on ovirt branch on your f.d.o repo). Rest we handle once remaining issues/questions have been addressed.
Attachment 227252 [details] pushed as 453b2f2 - Add generic broker support to App Attachment 227254 [details] pushed as 2d7dcf4 - libvirt: Move App::add_domain to LibvirtBroker Attachment 234833 [details] pushed as dd20c5d - build-sys: Pass --target-glib to valac
Created attachment 236043 [details] [review] ovirt: Conditionally use libgovirt during build
Created attachment 236044 [details] [review] Alphabetically order vala package checks
Created attachment 236045 [details] [review] ovirt: Add oVirt broker For now, the broker isn't doing much, but more will be added in later commits
Created attachment 236046 [details] [review] ovirt: Allow creating oVirt sources We expect an ovirt:// URI scheme and turn that into an https:// URI. There is no support for asking the user for authentication credentials at the moment.
Created attachment 236047 [details] [review] spice: Allow to set a SPICE TLS port The SpiceDisplay class did not support TLS ports, but oVirt uses TLS connections to VMs most of the time so SpiceDisplay needs to be extended to cope with this..
Created attachment 236048 [details] [review] ovirt: Implement OvirtMachine This is used to control oVirt VMs.
Created attachment 236049 [details] [review] ovirt: Add a way to set the SPICE host subject In some circumstances, the certificate the SPICE host we are connecting to will not be valid for the IP we are connecting to. When this happens, a 'host subject' has to be provided to spice-gtk so that it can authenticate the host it's connecting to. This has been implemented very recently in oVirt 3.1 REST API, see https://bugzilla.redhat.com/show_bug.cgi?id=807384 but requires some non-trivial work in libgovirt. In the mean time, add a BOXES_SPICE_HOST_SUBJECT environment variable for the cases when it's required, but it's really a hack and should be solved by using this new REST API. The host subject looks like O=organization,CN=hostname, where organization is an arbitrary organization name, and hostname is the hostname/IP the certificate was generated for. If the host address hasn't changed, its IP should work. It can happen that the host address isn't the SPICE address we are connecting to since oVirt can have different management/display addresses.
Created attachment 236050 [details] [review] searchbar: Allow to disable key press snooping The searchbar grabs all key presses in the collection view and use them to filter the displayed VMs. However, if we need keyboard input in another widget, the searchbar will prevent it. This commit adds methods to enable/disable this keyboard snooping when needed.
Created attachment 236051 [details] [review] ovirt: Add authentication dialog Ask the user for credentials if the remote oVirt instance needs authentication. This needs a librest version with support for the RestProxy::authenticate signal.
Created attachment 236052 [details] [review] machine: Fix some methods visibility Some methods from the Machine class were public while they are not really useful in the public interface. Mark these as protected or private.
Created attachment 236053 [details] [review] ovirt: Implement OvirtMachine::take_screenshot This makes it possible to reuse the machinery which takes regular box screenshot. This implementation of take_screenshot is arguably not really useful since screenshots will only be taken while the box is maximized/fullscreen, and the disconnect_display () logic takes care of taking a screenshot just before going back to the collection view. Taking regular screenshots can have its use in case of an abnormal gnome-boxes termination.
Created attachment 236054 [details] [review] CollectionView: Mark internal method as private The only user of this method is in CollectionView.
Created attachment 236055 [details] [review] notificationbar: Fix infinite loop in cancel()
You forgot to obsolete patches. -5 points.
Review of attachment 236043 [details] [review]: ack
Review of attachment 236044 [details] [review]: ack
Review of attachment 236045 [details] [review]: ack
Review of attachment 236046 [details] [review]: ack
Review of attachment 236047 [details] [review]: ack
Review of attachment 236048 [details] [review]: ack with these small things fixed. ::: configure.ac @@ +53,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.1.5 LIBXML2_MIN_VERSION=2.7.8 +SPICE_GTK_MIN_VERSION=0.15 I guess you forgot this comment from previous review: "If you could mention this bump in commit log, I won't ask you to put this in separate patch. :)" ::: src/ovirt-broker.vala @@ +1,3 @@ // This file is part of GNOME Boxes. License: LGPLv2+ using Ovirt; +using Gtk; Same here.
Review of attachment 236049 [details] [review]: ACK
Review of attachment 236050 [details] [review]: ack
Review of attachment 236051 [details] [review]: ack
Review of attachment 236052 [details] [review]: ack
Review of attachment 236053 [details] [review]: ack
Review of attachment 236054 [details] [review]: ack
Review of attachment 236055 [details] [review]: ack
(In reply to comment #202) > You forgot to obsolete patches. -5 points. Sorry, not interested in wasting my time because of our use of an inappropriate tool.
(In reply to comment #216) > (In reply to comment #202) > > You forgot to obsolete patches. -5 points. > > Sorry, not interested in wasting my time because of our use of an inappropriate > tool. Your thoughts on our choice of tool are not helping. Many other GNOME projects are using bugzilla for patches too and they are all happy so it can't possibly be that inappropriate a tool. Now please, update the status of the patches or someone else will have to do that for you and all you'll achieve is annoying people. Surely you don't want that.
(In reply to comment #217) > (In reply to comment #216) > > (In reply to comment #202) > > > You forgot to obsolete patches. -5 points. > > > > Sorry, not interested in wasting my time because of our use of an inappropriate > > tool. > > Your thoughts on our choice of tool are not helping. Many other GNOME projects > are using bugzilla for patches too and they are all happy so it can't possibly > be that inappropriate a tool. > > Now please, update the status of the patches or someone else will have to do > that for you and all you'll achieve is annoying people. Surely you don't want > that. Oh cool, you already did that. Thanks! From your last comment, it would seem like you were refusing to do it.
I can't help myself but wonder if teuf is using "git bz attach -e" which makes this process absolutely painless.
(In reply to comment #218) > > Oh cool, you already did that. Thanks! From your last comment, it would seem > like you were refusing to do it. If someone did it, that was not me, so thanks too.
(In reply to comment #219) > I can't help myself but wonder if teuf is using "git bz attach -e" which makes > this process absolutely painless. I'm just using git bz attach origin/master If adding a -e makes it easier to obsolete patches, I'll have to remember to try it.
Attachment 236043 [details] pushed as 2040b63 - ovirt: Conditionally use libgovirt during build Attachment 236044 [details] pushed as 036b2a7 - Alphabetically order vala package checks Attachment 236045 [details] pushed as 0ac4e22 - ovirt: Add oVirt broker Attachment 236046 [details] pushed as 7be0e61 - ovirt: Allow creating oVirt sources Attachment 236047 [details] pushed as f558eab - spice: Allow to set a SPICE TLS port Attachment 236048 [details] pushed as 8413cf3 - ovirt: Implement OvirtMachine Attachment 236049 [details] pushed as 7866102 - ovirt: Add a way to set the SPICE host subject Attachment 236050 [details] pushed as e33f5d6 - searchbar: Allow to disable key press snooping Attachment 236051 [details] pushed as 87f7345 - ovirt: Add authentication dialog Attachment 236052 [details] pushed as 35d52d6 - machine: Fix some methods visibility Attachment 236053 [details] pushed as 96b285d - ovirt: Implement OvirtMachine::take_screenshot Attachment 236054 [details] pushed as 5db84bf - CollectionView: Mark internal method as private Attachment 236055 [details] pushed as 7a93363 - notificationbar: Fix infinite loop in cancel()
Yay!