See https://bugzilla.gnome.org/show_bug.cgi?id=667593#c0 for why we don't setup a password on connection and rest of the discussion on that bug is relevant too.

Some pointers for implementation:

http://lists.freedesktop.org/archives/spice-devel/2011-October/005834.html
http://libvirt.org/html/libvirt-libvirt.html#virDomainOpenGraphics

The following patch is in queued for qemu to allow port-less servers:
http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01661.html

In the meatine, Boxes can learn to attach to local spice server via virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk port connect()).

(In reply to comment #3)
>
> In the meatine, Boxes can learn to attach to local spice server via
> virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> port connect()).

I thought that is as easy as calling virDomainOpenGraphicsFD() replacing spice_session_connect with spice_session_open_fd(), passing it the fd from former call but seems its freaking rocket science that doesn't seem to be well documented at all. :(

(In reply to comment #4)
> (In reply to comment #3)
> >
> > In the meatine, Boxes can learn to attach to local spice server via
> > virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> > port connect()).
> 
> I thought that is as easy as calling virDomainOpenGraphicsFD() replacing
> spice_session_connect with spice_session_open_fd(), passing it the fd from
> former call but seems its freaking rocket science that doesn't seem to be well
> documented at all. :(

Do you have specific question? This API can't be made more simple, because spice needs several file descriptor. You initiate the connection with session_open_fd(), this when channels are created SpiceSession::channel-new, you connect to the Channel::open-fd. When ::open-fd is emitted, you open a new connection to server and handle it back to the channel with spice_channel_open_fd().

Created attachment 290948 [details] [review]
spice-display: Micro readability improvement to a comment

ctor => constructor

Created attachment 290949 [details] [review]
display: Allow connecting through FD

Implementation classes can now support connecting through a file
descriptor.

Created attachment 290950 [details] [review]
spice-display: Allow connecting through FD

Make use of the new superclass API to allow users of this class to
connect to display using a file descriptor.

Created attachment 290951 [details] [review]
libvirt-machine: Connect using FD if local

If its a local libvirt machine, connect to SPICE display using file
descriptor. While the issue of spice connection being exposed to other
local users is not resolved by this patch[1], it prepares Boxes for the
real solution.

[1] Currently libvirt+Qemu mandate specifying a TCP port for the SPICE
display.

Created attachment 290952 [details] [review]
This patch will actually be coming before others:

Require libvirt-glib >= 0.2.0

This is to be able to use the newly added gvir_domain_open_graphics_fd()
function.

(In reply to comment #10)
> Created an attachment (id=290952) [details] [review]
> This patch will actually be coming before others:
> 
> Require libvirt-glib >= 0.2.0
> 
> This is to be able to use the newly added gvir_domain_open_graphics_fd()
> function.

newly added => just sent the patch to libvirt list. :)

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > >
> > > In the meatine, Boxes can learn to attach to local spice server via
> > > virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> > > port connect()).
> > 
> > I thought that is as easy as calling virDomainOpenGraphicsFD() replacing
> > spice_session_connect with spice_session_open_fd(), passing it the fd from
> > former call but seems its freaking rocket science that doesn't seem to be well
> > documented at all. :(
> 
> Do you have specific question?

I asked on IRC but you started ignoring me (you were answering other people) and documentation is pretty bad, hence the outrage here. :)

> This API can't be made more simple, because
> spice needs several file descriptor. You initiate the connection with
> session_open_fd(), this when channels are created SpiceSession::channel-new,
> you connect to the Channel::open-fd. When ::open-fd is emitted, you open a new
> connection to server and handle it back to the channel with
> spice_channel_open_fd().

That is fine and as you can see from attached patches, I figured it out in the end (I missed your comment) but I think all this should be explained in the docs. Here is what docs say about spice_session_open_fd:

"Open the session using the provided fd socket file descriptor. This is useful if you create the fd yourself, for example to setup a SSH tunnel.

If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd signal."

This gives the impression that if you pass this call a valid fd, you don't need to deal with SpiceChannel::open-fd.

(In reply to comment #12)
> > Do you have specific question?
> 
> I asked on IRC but you started ignoring me (you were answering other people)
> and documentation is pretty bad, hence the outrage here. :)

Eh? I don't recall that.

> "Open the session using the provided fd socket file descriptor. This is useful
> if you create the fd yourself, for example to setup a SSH tunnel.
> 
> If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd
> signal."
> 
> This gives the impression that if you pass this call a valid fd, you don't need
> to deal with SpiceChannel::open-fd.

Sorry if the documentation isn't giving you the solution to all your questions. I agree it could be improved. But complaining here is not helping much.

Review of attachment 290948 [details] [review]:

trivial, ack

Review of attachment 290949 [details] [review]:

ack

Review of attachment 290950 [details] [review]:

ack

Review of attachment 290951 [details] [review]:

you should bump libvirt dependency here

Review of attachment 290952 [details] [review]:

should be merge with previous patch

Review of attachment 290951 [details] [review]:

> you should bump libvirt dependency here

I tend to keep dep changes into separate commits so that I don't miss them in release notes.

Review of attachment 290951 [details] [review]:

but yeah, I agree the version bump patch should be parent of this one.

(In reply to comment #13)
> (In reply to comment #12)
> > > Do you have specific question?
> > 
> > I asked on IRC but you started ignoring me (you were answering other people)
> > and documentation is pretty bad, hence the outrage here. :)
> 
> Eh? I don't recall that.

Honest mistake then. nm :)

> > "Open the session using the provided fd socket file descriptor. This is useful
> > if you create the fd yourself, for example to setup a SSH tunnel.
> > 
> > If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd
> > signal."
> > 
> > This gives the impression that if you pass this call a valid fd, you don't need
> > to deal with SpiceChannel::open-fd.
> 
> Sorry if the documentation isn't giving you the solution to all your questions.
> I agree it could be improved. But complaining here is not helping much.

Agreed. I should send a patch instead.

Attachment 290948 [details] pushed as cb00668 - spice-display: Micro readability improvement to a comment
Attachment 290949 [details] pushed as af7f876 - display: Allow connecting through FD
Attachment 290950 [details] pushed as 9268e02 - spice-display: Allow connecting through FD
Attachment 290951 [details] pushed as fb04e17 - libvirt-machine: Connect using FD if local

I'm resolving this bug since everything on our side is hopefully done with these patches.

Something like

diff --git a/src/vm-configurator.vala b/src/vm-configurator.vala
index 9549682..027720b 100644
--- a/src/vm-configurator.vala
+++ b/src/vm-configurator.vala
@@ -72,7 +72,7 @@ public static Domain create_domain_config (InstallerMedia install_media, string
         install_media.setup_domain_config (domain);
         var graphics = new DomainGraphicsSpice ();
-        graphics.set_autoport (true);
+        graphics.set_autoport (false);
         graphics.set_image_compression (DomainGraphicsSpiceImageCompression.OFF);
         domain.add_device (graphics);


is missing as otherwise SPICE will still be listening on a local TCP port even though you are using a FD for the connection. I wonder if existing VMs created by Boxes should be upgraded to so that they stop listening on localhost.

Created attachment 324497 [details] [review]
Spice connection exposed to other users

I put graphics.set_autoport(false);

This should make spice not listening on local TCP port.

Bug number 738573

Review of attachment 324497 [details] [review]:

* Please read this guideline and try your level best to follow it: https://wiki.gnome.org/Git/CommitMessages

* "I put graphics.set_autoport(false);"

You want to describe the change(s) in English here, not repeat the code.

* "This should make". Since it's easy to test this, I don't think we need to speculate here. So you'd write "This makes".

* You also want to mention that some of the changes (with commit hash of the change, preferably) in this regard have already been merged but this change is still needed.

* In Boxes project, we use the bug URL at the end (not just the number) for reference. So it'll be:

https://bugzilla.gnome.org/show_bug.cgi?id=738573

::: src/vm-configurator.vala
@@ +71,3 @@
         set_target_media_config (domain, target_path, install_media);
         install_media.setup_domain_config (domain);
+	

Irrelevant change. Please remove this change.

Created attachment 326505 [details] [review]
vm-configurator: Add add_graphics_device()

Refactor code that adds a new graphics device into a separate method.

Created attachment 326506 [details] [review]
vm-configurator: Don't autoport spice in new VMs

If we set autoport=true on spice display in new VMs, spice will still
expose the VM's display on a local TCP port even if Boxes itself
connnects to FD socket.

Created attachment 326507 [details] [review]
vm-configurator: Don't expose SPICE in existing VMs

Update configuration of existing local VMs on launch to not expose SPICE
connection on TCP so other users can't connect to the display.

Created attachment 326508 [details] [review]
spice-display: Special constructor for private connection

If SPICE is not exposed on TCP, we end up with SpiceDisplay constructor
failing due to port and tls_port both being 0. Let's add a special
constructor for the case of local private SPICE connection.

Created attachment 326509 [details] [review]
display: uri property now nullable

In some cases URI won't make sense so let's make it nullable. One such
case is that of local private VMs where we connect through an FD and a
following patch will modify uri in SpiceDisplay null for this case.

Created attachment 326510 [details] [review]
spice-display: No URI for local private case

In case of local private connection, a URI is irrelevant so let's not
ask Spice.Session to create a URI in this case as it'll fail to do so.

Attachment 326505 [details] pushed as c53d60c - vm-configurator: Add add_graphics_device()
Attachment 326506 [details] pushed as c723802 - vm-configurator: Don't autoport spice in new VMs
Attachment 326507 [details] pushed as 58c8e90 - vm-configurator: Don't expose SPICE in existing VMs
Attachment 326508 [details] pushed as 77a24a3 - spice-display: Special constructor for private connection
Attachment 326509 [details] pushed as 0a0a2c2 - display: uri property now nullable
Attachment 326510 [details] pushed as 0e74f8a - spice-display: No URI for local private case