(In reply to comment #0)
> In the Properties view you can read the spice URI, which contains part of the
> infromation enables you to connect to that box from another machine. On my F18
> box, for instance, it says 'spice://127.0.0.1?port=5901;'. Is it possible to
> provide the external IP address, and a description expressing the fact that I
> can connect to this box from Boxes on another machine using this address?
> Zeeshan demonstrated this at the hackfest, but in order to get the correct port
> number (6400, I think), he needed to parse an external XML file.

Actually I needed to edit the xml. Thing is that currently we keept the VMs local only so by default you can't connect to them. Also the ports are dynamic. So what I did was to enable spice server on external IP and assign it a fixed port . Don't know why 5900 was already used on your machine cause there was only one box, perhaps some machines running outside of Boxes control/view?

(In reply to comment #0)
> In addition,
> we were unable to get the external IP address from Network Settings, so it
> would be nice to see it here.

*the* external IP address? you can configure qemu to listen on all external addresses by using 0.0.0.0 (think wifi + ethernet), so we don't necessarily have an external IP to show there. What could be nice is advertising the spice connection through avahi (?)

*** Bug 666365 has been marked as a duplicate of this bug. ***

*** Bug 758839 has been marked as a duplicate of this bug. ***

Created attachment 330923 [details] [review]
vm-configurator: Expose SPICE connections on network

Created attachment 330924 [details] [review]
Add avahi dependencies

Boxes is going to use avahi-client and avahi-gobject to advertise
SPICE connections on the network.

Review of attachment 330923 [details] [review]:

::: src/vm-configurator.vala
@@ +502,3 @@
+                                                          <listen type='address' address='0.0.0.0'/>
+                                                          <image compression='off'/>
+                                                          </graphics>");

* It's not as simple as that. :) We want the SPICE connection to be private by default but provide a way in properties to expose it. In fact it was very recently that we made the SPICE connection completely private (before, it was still exposed to users on the same network).

* Avoid writing xml whenever possible. There is a reason why libvirt-gconfig exists.

Review of attachment 330924 [details] [review]:

Good that you made this a separate patch but not much point of this patch until we actually use avahi.

Review of attachment 330923 [details] [review]:

::: src/vm-configurator.vala
@@ +502,3 @@
+                                                          <listen type='address' address='0.0.0.0'/>
+                                                          <image compression='off'/>
+                                                          </graphics>");

Altering the XML file in properties doesn't appear to be an option, because the machine needs to be restarted for the changes to occur. Do you know any other possible approach ? I'll start researching tomorrow.

The problem is that the API for GVirConfig.DomainGraphicsSpice doesn't have the option to change the listening IP, so writing XML may be our last resort.

(In reply to Visarion Alexandru from comment #9)
> The problem is that the API for GVirConfig.DomainGraphicsSpice doesn't have
> the option to change the listening IP, so writing XML may be our last resort.

Patches adding whatever API you need to libvirt-glib would be considered and very welcome :)

Review of attachment 330923 [details] [review]:

::: src/vm-configurator.vala
@@ +502,3 @@
+                                                          <listen type='address' address='0.0.0.0'/>
+                                                          <image compression='off'/>
+                                                          </graphics>");

* There is a few other XML changes (e.g disk and memory resize) that also requires  a reboot. Check how they are handled.

* As Christophe said, you need to add the API then. libvirt-gconfig was mostly written for Boxes and we add API for Boxes use in it all the time.

Created attachment 331001 [details] [review]
libvirt-machine: Advertise SPICE connections

If we want to let the user expose SPICE connections for local
machines, we should first make sure that connecting to them
isn't cumbersome.

Let's use Avahi for advertising the external IP address and the
port of the connection, alongside a description text.

(In reply to Visarion Alexandru from comment #12)
> Created attachment 331001 [details] [review] [review]
> libvirt-machine: Advertise SPICE connections
> 
> If we want to let the user expose SPICE connections for local
> machines, we should first make sure that connecting to them
> isn't cumbersome.
> 
> Let's use Avahi for advertising the external IP address and the
> port of the connection, alongside a description text.

I wonder if this shouldn't be in spice-server or libvirt instead. This isn't going to advertize the VM unless Boxes is running, right?

Review of attachment 331001 [details] [review]:

I'd divide this into two patches, one to add network-publisher class and the other to start using it.

Next two changes will be:

* Add UI to expose SPICE display on the network. I think this should come first (before this patch) actually but no strong feelings.

* Automatically create and destroy boxes for SPICE connections on local network, as they appear and disappear, respectively.

::: src/app.vala
@@ -130,2 @@
         collection = new Collection ();
-

Redudnant change.

::: src/libvirt-machine.vala
@@ +419,3 @@
         switch (type) {
         case "spice":
+            if (port > 0) {

* As we discussed, we only want to advertise SPICE connection that is local.
* All this code should go into a separate method.

@@ +422,3 @@
+                ulong state_machine_id = 0;
+
+                if (!App.app.network_publisher.has_service (name)) {

The user code should not have to check for this condition. NetworkPublisher.publish_service() should take care of it.

@@ +423,3 @@
+
+                if (!App.app.network_publisher.has_service (name)) {
+                    App.app.async_launcher.launch.begin ( () => {

Why do we need to launch them async?

::: src/network-publisher.vala
@@ +4,3 @@
+
+    private string domain = "";
+    private string host = "";

* We don't need instance fields for empty strings.
* I assume empty host/domain means avahi takes care of these?

@@ +6,3 @@
+    private string host = "";
+    private string type = "_spice._tcp";
+    private string text = "Boxes SPICE connection.";

* constants should be declared as such, and before non-constants.

* While the class is generically named and is generic in other ways, these makes it SPICE-specific. I think we want to keep it generic. Let the user of this API pass both to us.

Please ensure that user still doesn't get exposed to any avahi details. So define a constant for "_spice._tcp" (if later we advertise another protocol, we can create another constant for that) and let user just pass that to us. Also 'text' won't be a good name, so expose it as 'description'.

@@ +16,3 @@
+    private Mutex data_mutex;
+    private Mutex established_mutex;
+    private Cond established_cond;

Why do you need mutexes and locks? We are not doing multithreading here.

@@ +18,3 @@
+    private Cond established_cond;
+
+    public static NetworkPublisher get_default () {

This doesn't need to be a singleton if all code can access the instance from App.app singleton.

@@ +31,3 @@
+
+        names = new List<string> ();
+        ports = new List<uint16> ();

Instead of having multiple lists, you want a small class and then just keep a list of instances of that class.

Or

You could just have a HashTable mapping names to ports.

@@ +57,3 @@
+    }
+
+    public void remove_service (string service_name, uint16 service_port) {

* port shouldn't be required here.
* This function will be a few lines if you use a HashTable instead and remove the redundant locking/mutex.

@@ +65,3 @@
+
+        if (names.length () > 0)
+            refresh_services ();

So instead of unpublishing the service in question, you unpublish all services and then re-add the remainaing ones in the list? That's pretty bad and will cause unnecessary network traffic. Just unpublish this service. Pretty sure Avahi provides API for that.

Same goes for publish_service(). You should simply ditch refresh_services cause it's a bad idea.

@@ +86,3 @@
+            for (int i = 0;i < names.length ();i++) {
+                group.add_service_full (Interface.UNSPEC, Protocol.UNSPEC,  0,
+                                       names.nth_data (i), type, domain, host, ports.nth_data (i), text);

coding style: Each arg on separate line if splitting function call into multiple lines.

@@ +95,3 @@
+            established_mutex.unlock ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

Would be nice to append some context here.

@@ +103,3 @@
+            group.reset ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

context in warning message here too.

@@ +111,3 @@
+            established_cond.signal ();
+        else if (state == EntryGroupState.FAILURE)
+            debug ("Failed to publish connections");

I think this should be a warning.

(In reply to Marc-Andre Lureau from comment #13)
> (In reply to Visarion Alexandru from comment #12)
> > Created attachment 331001 [details] [review] [review] [review]
> > libvirt-machine: Advertise SPICE connections

> I wonder if this shouldn't be in spice-server or libvirt instead. This isn't
> going to advertize the VM unless Boxes is running, right?

Yes, it advertises only if one can actually connect to the VM (when it is running).

(In reply to Zeeshan Ali (Khattak) from comment #14)
> Review of attachment 331001 [details] [review] [review]:
>
> * Automatically create and destroy boxes for SPICE connections on local
> network, as they appear and disappear, respectively.

Actually I talked to jimmac about this and realised this is not a good idea. We should instead be making it easy to find/add these in wizard. There is actually a page and mockups for this:

https://wiki.gnome.org/Design/Apps/Boxes/RemoteConnections

Review of attachment 331001 [details] [review]:

::: src/network-publisher.vala
@@ +95,3 @@
+            established_mutex.unlock ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

Here are some examples of warning messages: 

(gnome-boxes:18128): Boxes-WARNING **: network-publisher.vala:32: Committing group failed: Is empty 
(gnome-boxes:18128): Boxes-WARNING **: network-publisher.vala:24: Adding service to group failed: Invalid service type

Don't they already have enough context ?

Review of attachment 331001 [details] [review]:

::: src/network-publisher.vala
@@ +95,3 @@
+            established_mutex.unlock ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

Here are some examples of warning messages: 

(gnome-boxes:18128): Boxes-WARNING **: network-publisher.vala:32: Committing group failed: Is empty 
(gnome-boxes:18128): Boxes-WARNING **: network-publisher.vala:24: Adding service to group failed: Invalid service type

Don't they already have enough context ?

Review of attachment 331001 [details] [review]:

::: src/network-publisher.vala
@@ +95,3 @@
+            established_mutex.unlock ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

(gnome-boxes:18128): Boxes-WARNING **: network-publisher.vala:32: Committing group failed: Is empty 

What group? What were you trying to do that failed here? That would be the context:

g_warning ("Failed to publish service "%s": %s", name, error.message);

The error thrown can change from the thrower so caller should also add it's context in most cases.

Created attachment 331224 [details] [review]
Add avahi dependencies

Boxes is going to use avahi-gobject to advertise SPICE connections
on the network.

Created attachment 331226 [details] [review]
Add NetworkPublisher

If we want to let the user expose SPICE connections for local
machines, we should make sure that connecting to them isn't
cumbersome.

Let's create a class that uses Avahi to handle advertising the
external IP address and the port of the connection, alongside
a description text.

Created attachment 331227 [details] [review]
libvirt-machine: Advertise local SPICE connections

Let's publish on the network the local SPICE connections of
libvirt machines, in order for other GNOME-Boxes users on the
same network to be able to automatically connect to them.

Review of attachment 331001 [details] [review]:

::: src/network-publisher.vala
@@ +4,3 @@
+
+    private string domain = "";
+    private string host = "";

Yes, avahi takes care of these.

Created attachment 331228 [details] [review]
Add NetworkPublisher

If we want to let the user expose SPICE connections for local
machines, we should make sure that connecting to them isn't
cumbersome.

Let's create a class that uses Avahi to handle advertising the
external IP address and the port of the connection, alongside
a description text.

Review of attachment 331224 [details] [review]:

Has this patch changed? If the patch doesn't change and it's at the beginning of the patch and has been marked "commitnow", no need to re-upload it. It only wastes reviewer's time.

Review of attachment 331227 [details] [review]:

Ideally no patch should depend on the next patch for building. This patch will break the build without next patch. You want the next patch before this one.

Good stuff.

::: src/app.vala
@@ +41,3 @@
     public CollectionSource default_source { get { return sources.get (DEFAULT_SOURCE_NAME); } }
     public AsyncLauncher async_launcher;
+    public NetworkPublisher network_publisher;

Changes to App aren't really fitting to this patch. I'd either make them separate patch, that comes before this one (most preferred) or move them to patch adding NetworkPublisher class.

@@ +270,3 @@
         var display = Gdk.Display.get_default ();
         display.flush ();
+        network_publisher.unpublish_all ();

Have you checked if Avahi doesn't automatically does that for you if the process exits? Knowing project was created by Lennart, I'd guess that it does that for you. :)

::: src/libvirt-machine.vala
@@ +421,3 @@
+            if (port > 0) {
+                if (host != "localhost" && host != "127.0.0.1")
+                    publish_connection ((uint16)port);

coding style nitpick: Space between (uint) and port.

@@ +833,3 @@
+
+        state_machine_id = notify["state"].connect (() => {
+            if (state != MachineState.RUNNING) {

No need for curly braces around single blocks, unless their lack reduces readability, which it doesn't in here.

@@ +837,3 @@
+            }
+
+

* You are disconnecting regardless of the state. This will fail for sure on fake/redundant state changes for example. i-e on a state change to RUNNING even though machine was already running.

* We are publishing on connecting to display, so it makes sense to unpublish on disconnection of display rather than state of machine.

Review of attachment 331228 [details] [review]:

Good work on commit log but here you don't need to justify export of SPICE connection as justification for this patch. Just say that we'll be using this class in a following patch to advertise SPICE connections on the local network and that is a good justification for this patch. You can also mention that this class is kept generic so that in future it's easy to advertise other services, if needed.

::: src/network-publisher.vala
@@ +3,3 @@
+using Avahi;
+
+private class Service {

Since this is just a private class to NetworkPublisher, please define if after the NetworkPublisher.

@@ +6,3 @@
+    public string service_name { get; private set; }
+
+    private EntryGroup group;

Except for Boxes and GLib namespace, for last two years I've been always been favouring explicit namespaces so I'd prefer if you do too so that least all new code is consistent and more readable in this regard. I should change all code but too lazy to do that. :)

@@ +53,3 @@
+
+public class NetworkPublisher {
+    public static const string SPICE_TYPE = "_spice._tcp";

* name is pretty vague. Let's do SERVICE_TYPE_SPICE.

* I know I recommended doing it exactly this way but now that I think more about it, let's do it even more nicer: define an enum for service types and accept that as parameter and translate that to a string internally. This makes it impossible for caller to pass incorrect data (for example in future someone tries to publish another service, makes a typo and ends up wasting a lot of their time debugging that). For example on how to do this: see Boxes.WizardPage enum and Wizard.page_names usage.

@@ +66,3 @@
+            client.start ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

Context in warning.

@@ +76,3 @@
+
+        Service service = new Service (client, service_name, service_type, service_port, description);
+        service.publish ();

Since we always publish the service, I don't see why caller should need to call .publish() explicitly.

@@ +81,3 @@
+    }
+
+    public void remove_service (string service_name) {

naming is not consistent with other two methods' names.

@@ +99,3 @@
+    public void on_server_state_changed (ClientState state) {
+        if (state == ClientState.S_COLLISION)
+            warning ("Server collision.");

What does that mean? Sounds really scary. :) The message should give some clue to avahi-illiterate developer/user.

Review of attachment 331224 [details] [review]:

>Has this patch changed? If the patch doesn't change and it's at the beginning of the patch and has been marked "commitnow", no need to re-upload it. It only wastes reviewer's time.
At some point yesterday I thought we won't need avahi-client, just avahi-gobject. It turns out we do, for the sake of one method, so I'm gonna re-re-upload the previous one, with both avah-client and avahi-gobject. Sorry !

Review of attachment 331227 [details] [review]:

::: src/app.vala
@@ +41,3 @@
     public CollectionSource default_source { get { return sources.get (DEFAULT_SOURCE_NAME); } }
     public AsyncLauncher async_launcher;
+    public NetworkPublisher network_publisher;

If we don't need to unpublish all the services in the App class, we won't need the NetworkPublisher there anymore. We could just instantiate it locally in the LibvirtMachine class.

@@ +270,3 @@
         var display = Gdk.Display.get_default ();
         display.flush ();
+        network_publisher.unpublish_all ();

It does :)

::: src/libvirt-machine.vala
@@ +837,3 @@
+            }
+
+            disconnect (state_machine_id);

The idea behing it is that the machine should be advertised while it is running (while users can connect to it). There are cases (example: when it has no USB devices) when the machine is running but the display disconnects in overview mode.

My implementation had the right behaviour, but it looks more like a workaround now.
I think I should actually do both publishing and unpublishing depending on the state of the machine.

Created attachment 331314 [details] [review]
Add NetworkResolver

We need a class that will help us in further patches to create
Boxes out of advertised SPICE connections.

Let's create a generic class that uses Avahi to detect and resolve
advertised connections.

Created attachment 331315 [details] [review]
Add avahi dependencies

Boxes is going to use avahi-client and avahi-gobject to advertise
SPICE connections on the network.

Review of attachment 331227 [details] [review]:

::: src/app.vala
@@ +41,3 @@
     public CollectionSource default_source { get { return sources.get (DEFAULT_SOURCE_NAME); } }
     public AsyncLauncher async_launcher;
+    public NetworkPublisher network_publisher;

but we need the publisher object around so it'll need to be singleton then (as it was before).

@@ +270,3 @@
         var display = Gdk.Display.get_default ();
         display.flush ();
+        network_publisher.unpublish_all ();

Great. :)

::: src/libvirt-machine.vala
@@ +837,3 @@
+            }
+
+

If there are cases when machine is RUNNING but display is disconnected, I don't see how you come to the conclusion in the second para.

Review of attachment 331227 [details] [review]:

::: src/libvirt-machine.vala
@@ +837,3 @@
+            }
+
+            disconnect (state_machine_id);

I came to the conclusion that we should publish/unpublish based on machine state because the machine should be advertised while it is in RUNNING state (if the user wants to advertise it, why not advertise it as much as possible, and not only when display is connected?)

We advertise it solely because we want other users to connect to it. Why should we need the display to be connected in order for other people to see it on the network? 

My implementation had the same behaviour because it would start advertising at the first display connection (when machine enters RUNNING state) and it would stop when machine exited RUNNING state.

Review of attachment 331227 [details] [review]:

::: src/libvirt-machine.vala
@@ +837,3 @@
+            }
+
+

"why not advertise it as much as possible, and not only when display is connected?"

Because we are advertising the display as an available service, not the machine. It doesn't make any sense to advertise local machines.

Created attachment 331525 [details] [review]
spice-display: Add is_exposed property

Add a boolean property which is true if the display is
exposed on the network and false otherwise.

Created attachment 331526 [details] [review]
libvirt-machine-properties: Let the user expose machine

Let the user choose whether he wants to share the display of
a local libvirt machine on the network.

Currently available only for local machines with SPICE display.

Created attachment 331529 [details] [review]
Bump libvirt-glib dependency

Boxes will need to use a method for changing the address
a domain is listening to, which was introduced in 0.2.4

Created attachment 331562 [details] [review]
Add NetworkPublisher

Add a generic class that uses Avahi to handle advertising the
external IP address and the port of a connection, alongside
a description text.

We will use this to advertise SPICE connections on the local
network.

Created attachment 331566 [details] [review]
spice-display: Handle advertising own connection

Make SPICE display capable of publishing its connection on
the network.

Make SPICE display unpublish its connection when disconnecting.

We will use this to advertise SPICE connections of local libvirt
machines.

Created attachment 331567 [details] [review]
libvirt-machine: Advertise local SPICE connections

Let's publish on the network the SPICE connections of local
libvirt machines, in order for other GNOME-Boxes users on
the same network to be able to automatically connect to them.

Review of attachment 331525 [details] [review]:

::: src/display.vala
@@ +15,3 @@
     public string? username { get; set; }
     public bool connected;
+    public virtual bool is_exposed { get { return false; } }

* Better move it above "connected" so props are listed together.

* If it's going to be in the generic class, you should also implement this for vnc.

::: src/spice-display.vala
@@ +14,3 @@
         }
     }
+    public override bool is_exposed { get { return uri != null && session.host != "" && session.host.substring (0, 3) != "127"; } }

* is_exposed is very vague, exposed to what? How about "exposed_on_network"?

* There is string.has_prefix() you can use.

Review of attachment 331526 [details] [review]:

* Been shortening the module name to "libvirt-machine-props" in shortlog to make it easier to fit in 50 chars, i'd suggest you do the same. :)

* How about "Allow exposing display on net" as shortlog?

* It's not just currently limited to local machines but that's what we want in future as well. Log makes it sound like a temporary limitation.

* I think we should make it work for VNC too, since it won't take a lot more to handle VNC as well.

::: src/libvirt-machine-properties.vala
@@ +138,3 @@
                     add_string_property (ref list, _("Display URL"), machine.display.uri);
+
+                if (machine.display.protocol != "SPICE")

* Being so specific here sucks and I'm pretty sure you can avoid this by use/addition of API in Display class.

* if you really need to do this, you should check for class: machine.display is SpiceDisplay.

@@ +161,3 @@
+                        // This will take effect only after next reboot
+                        machine.domain.set_config (config);
+                var property_share = add_property (ref list, _("Share Display"), toggle_connection);

This is misaligned from it's try.

::: src/vm-configurator.vala
@@ +548,3 @@
     }
 
+    public static void change_listen_address (Domain domain, string address) {

Addition of this method should go into a separate patch.

@@ +555,3 @@
+            if ((device is DomainGraphicsSpice)) {
+                graphics = device as DomainGraphicsSpice;
+                graphics.set_listen_address (address);

I might be wrong but I think you don't need to set a listening address at all, just set the port to a valid value or set autoport to true. AFAIK you set lisenting address if you want to restrict to specific network.

Review of attachment 331529 [details] [review]:

* This patch should come before the previous one.

* 0.2.4 is not out yet so you want to use future tense. Also for future ref, when uploading the patch, in the comment you should mention the status of the required patch to other component (libvirt-gconfig in this case) unless it's already merged.

Review of attachment 331562 [details] [review]:

Getting there!

::: src/network-publisher.vala
@@ +11,3 @@
+
+public class NetworkPublisher {
+    private const string[] service_types = { "_spice._tcp" };

convention is to always have constants in capital letters. I think the example from me you followed, didn't set a good example for this. :)

@@ +32,3 @@
+            client.start ();
+        } catch (Avahi.Error e) {
+            warning (e.message);

more context still wanted. :)

@@ +44,3 @@
+        services.append (service);
+
+        if (client.state == Avahi.ClientState.S_RUNNING)

Do we really need to take care of this? Shouldn't avahi publish the service when it's in running state again?

@@ +45,3 @@
+
+        if (client.state == Avahi.ClientState.S_RUNNING)
+            service.publish ();

As I said in previous review of this, publishing should be explicit in Service constructor.

@@ +84,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            unpublish_all ();

Do we really need to do this ourselves? Surely avahi takes care of re-advertising the services when it goes into some non-running state?

@@ +97,3 @@
+}
+
+private class Service {

Let's call it AvahiService.

@@ +133,3 @@
+                                    description);
+            group.commit ();
+        } catch (Avahi.Error e) {

I think error should not be caught here but propagated and from NetworkPublisher, you should transform it into non-avahi error. Feel free to define an error domain (which is easy in Vala) for NetworkPublisher errors. This is so that caller of this method knows publishing failed and it can take appropriate action.

@@ +141,3 @@
+        try {
+            group.reset ();
+        } catch (Avahi.Error e) {

Same about this error.

Review of attachment 331526 [details] [review]:

::: src/libvirt-machine-properties.vala
@@ +138,3 @@
                     add_string_property (ref list, _("Display URL"), machine.display.uri);
+
+                if (machine.display.protocol != "SPICE")

If we are going to implement it for VNC as well, we might as well just discard this check.

::: src/vm-configurator.vala
@@ +555,3 @@
+            if ((device is DomainGraphicsSpice)) {
+                graphics = device as DomainGraphicsSpice;
+                graphics.set_listen_address (address);

Actually, if I only set autoport to true, the host defaults to "127.0.0.1", so it won't be listening for connections on the network.

Review of attachment 331562 [details] [review]:

::: src/network-publisher.vala
@@ +84,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            unpublish_all ();

I actually talked with the guys from avahi on irc about error handling.
They said the best way to implement service publishing is to just follow their official C example, which is here:

http://avahi.org/download/doxygen/client-publish-service_8c-example.html
 99.9%, the state of the client will be S_RUNNING, but you could end up in those other states.
I didn't ask them if avahi automatically publishes services again, because I figured they wouldn't have done it explicitly in their example if it did.

Created attachment 331701 [details] [review]
display: Add exposed_on_network property

Add a boolean abstract property which is true if the display
is exposed on the network and false otherwise.

Created attachment 331702 [details] [review]
vm-configurator: Handle changing listening address

We need to be able to change the address on which a SPICE
or VNC display is listening to, so that we can expose it on
the network or unexpose it.

Created attachment 331703 [details] [review]
libvirt-machine-props: Allow exposing display on net

Let the user choose whether he wants to share the SPICE or VNC
display of a local libvirt machine on the network.

Review of attachment 331529 [details] [review]:

>This patch should come before the previous one. 

Before  "Add avahi dependencies" ?

Review of attachment 331526 [details] [review]:

::: src/vm-configurator.vala
@@ +555,3 @@
+            if ((device is DomainGraphicsSpice)) {
+                graphics = device as DomainGraphicsSpice;
+                graphics.set_listen_address (address);

Ah ok. Just ensure that's the case. :)

Review of attachment 331529 [details] [review]:

No, nm. Either the order of patches was incorrect when I commented on this patch or I got confused. The other point is still valid though.

Review of attachment 331315 [details] [review]:

Well at least this patch should come before the previous "Add NetworkResolver".

Review of attachment 331314 [details] [review]:

This patch will break the build cause it comes before the patch that adds avahi deps.

::: src/network-resolver.vala
@@ +9,3 @@
+
+    public signal void new_service (string name, string address, int port);
+    public signal void removed_service (string name);

I'm not sure I understand your API design here. Signals are usually used for commincation from class/object to it's users and not the other way around. There are exceptions to this rule but I doubt they are justified in this context. There were some issues with your previous API but the approach was sound.

@@ +29,3 @@
+    }
+
+    public void on_resolved_service (Interface @interface, Protocol protocol, string name, string type, string domain, string hostname, Address? address, uint16 port, StringList? txt) {

* You sure this line fits 120 chars? Probably a good idea to break it into multiple anyway. Same is true for on_removed_service.

* on_service_resolved() sounds better.

* Are are these signal handlers public?

Review of attachment 331566 [details] [review]:

* "Handle" makes it sound like it was supposed to do that already. Just "Advertise connection on network" suffice.

* The description is good but first two sentences sound like a robot and no need to put every sentence in separate para. Does this sound better?

"This patch makes SPICE display capable of publishing itself on
the network. The display automatically unpublishes itself on disconnection."

* Best to append " in a following patch" to last para.

::: src/spice-display.vala
@@ +427,3 @@
     }
 
+    public void publish_connection () {

I publishing is a decision for user of this class, then so should unpublishing be too. I think display should handle this all on it's own.

Review of attachment 331567 [details] [review]:

The log is good, but it gives the impression that this patch advertises the SPICE connection unconditionally.

::: src/libvirt-machine.vala
@@ +403,3 @@
+
+            if (display.is_exposed)
+                (display as SpiceDisplay).publish_connection ();

* I thought we'll make this generic and hence won't need to check the type or cast to specific type.

* It's a bit of a circular logic if display is telling us that we should expose it. If display knows it should be exposed, we shouldn't need to tell it that it should actually do it.

Review of attachment 331701 [details] [review]:

Oh, I'm guessing I just wasted my time reviewing previous patches cause you forgot to obsolete them? :( Some of the comments still apply though I think.

Review of attachment 331314 [details] [review]:

::: src/network-resolver.vala
@@ +9,3 @@
+
+    public signal void new_service (string name, string address, int port);
+    public signal void removed_service (string name);

It's actually my first version of the NetworkResolver class and it is the first time you review it. The other one was the NetworkPublisher. This is used to gather up connections from the network.

Both new_service and removed_service are used to communicate to the user that a new service has been found or that a service has been removed.
I do not use them internally. There are two signals with the same name which belong to the ServiceBrowser class. I am only creating callbacks for the ServiceBrowser's signals internally, although it would be a good idea to name my own signals differently.

Review of attachment 331315 [details] [review]:

>Well at least this patch should come before the previous "Add NetworkResolver".
Hmm, weird.Is it enough to have them in my local branch in the wright order for them to be here in the wright order ?

Review of attachment 331701 [details] [review]:

I honeslty don't know what makes you think so. Afaik there's no patch that should be obsolete and isn't.
All your reviews are on new patches, but please do reply to the discussion we have on the "Add NetworkPublisher" patch, as there is still some stuff left to discuss.

Review of attachment 331314 [details] [review]:

::: src/network-resolver.vala
@@ +9,3 @@
+
+    public signal void new_service (string name, string address, int port);
+    public signal void removed_service (string name);

Oh, I'm terribly sorry. I completely misunderstood this class. :( Your approach is correct then. I didn't realise you were already working on the client side of things and got it confused with the NetworkPublisher. Also the signal handlers being public confused me a bit.

Review of attachment 331315 [details] [review]:

>>Well at least this patch should come before the previous "Add NetworkResolver".
>Hmm, weird.Is it enough to have them in my local branch in the wright order for them to be here in the wright order ?

Bugzilla has no idea of your git branch or repository even, it just deals with patch files. So it all depends on the upload order. If you upload A and B and later upload A' that obsoletes A and you don't touch B, now B will be before A' since it as uploaded first. So in this case, you'd want to re-upload both even if you did not change B at all. OTOH, if you only change B, best to only upload B only. In any case, you might want to add a comment when uploading an upchanged patch so reviewers know. :)

Review of attachment 331701 [details] [review]:

Never mind what I said about obsoleting. As I said, I just got confused between NetworkPublisher and NetworkResolver.

Review of attachment 331562 [details] [review]:

::: src/network-publisher.vala
@@ +84,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            unpublish_all ();

You should ask them still if you can't figure on your own. The example is probably being very complete for demonstration purposes.

Created attachment 332635 [details] [review]
Bump libvirt-glib dependency

Boxes will need to use a method for changing the address
a domain is listening to, which will be introduced in 0.2.4.

Created attachment 332636 [details] [review]
Bump gtk+ dependency

Boxes will need to use a ScrolledWindow for the list of shared
connections in the wizard. This new version of gtk+ handles
the size of the ScrolledWindow much better than the previous
versions, so now we don't have to handle it ourselves.

Created attachment 332637 [details] [review]
display: Add exposed_on_network property

Add a boolean abstract property which is true if the display
is exposed on the network and false otherwise.

Review of attachment 332637 [details] [review]:

not modified

Review of attachment 331562 [details] [review]:

::: src/network-publisher.vala
@@ +84,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            unpublish_all ();

Turns out I was very lucky that I managed to reach them the first time :)
Tried on irc and email list, but no answers. I tried to figure out by myself but it's very hard by just looking at the code and not being able to reproduce.

I have already did some changes, so I don't have this version of this patch anymore. I'll remember to put this patch in the right order next time.

Created attachment 332639 [details] [review]
spice-display: Advertise connection on network

This patch makes a local SPICE display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

Created attachment 332640 [details] [review]
vnc-display: Add machine reference property

VNC-display needs this information so that it can be able
,in following patches, to publish its connection under the
name of its machine. This will also helps us detect local
machines, so that we can be able to publish only local displays.

Created attachment 332641 [details] [review]
vnc-display: Add machine constructor parameter and prop

VNC-display needs this property so that it can be able,
in following patches, to publish its connection under the
name of its machine. This will also helps us check the type
of the machine, so that we can be able to publish only local
displays.

Created attachment 332642 [details] [review]
vnc-display: Advertise connection on network

This patch makes a local VNC display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

Created attachment 332643 [details] [review]
vm-configurator: Handle changing listening address

We need to be able to change the address on which a SPICE
or VNC display is listening to, so that we can expose it on
the network or unexpose it.

Created attachment 332644 [details] [review]
libvirt-machine-props: Allow exposing display on net

Let the user choose whether he wants to share the SPICE or VNC
display of a local libvirt machine on the network.

Created attachment 332645 [details] [review]
Add NetworkResolver

We need a class that will help us in further patches to create
Boxes out of advertised SPICE connections.

Let's create a generic class that uses Avahi to detect and resolve
advertised connections.

Created attachment 332647 [details] [review]
wizard-source: Add WizardRemoteEntry class

In following patches we will implement a 'Remote' source page for
the wizard which will store all the remote connections discovered
on the network.

Let's add a class that represents one remote connections, both
visually and logically.

Created attachment 332649 [details] [review]
wizard-source: Add 'Remote' source page

Add a source page on which the user can choose between various
remote-connections found on the network by the NetworkResolver.

Created attachment 332656 [details] [review]
wizard: Handle 'Remote' source page

In a previous patch we added the 'Remote' source page. Let's
handle it in the wizard, to give it an appropriate behaviour.

Created attachment 332657 [details] [review]
wizard: Enable 'Preparation' and 'Setup' for remote boxes

Currently, when creating boxes out of VNC or SPICE uris, we do
not check the connectivity of the remote connections and we do
not try to log in, thus letting the user potentially create
useless boxes.

Let's stop skipping the 'Preparation' and 'Setup' phases for
remote connections, so that we can implement additional checks for
them in future patches.

Created attachment 332658 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise.

Created attachment 332659 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 332660 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 332661 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 332662 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 332663 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 332665 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise.

Created attachment 332666 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 332667 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 332668 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 332669 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 332670 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 332671 [details] [review]
wizard: Check remote connection credentials in 'Setup'

Currently, when creating a box out of a remote connection/uri
we do not check whether the user has the wright credentials to
connect to it, potentially letting the user create a box to
which he has no access.

Let's check that the user has the correct credentials to access
the machine and store them in the source of the machine, for
future usage.

Created attachment 332672 [details] [review]
machine: Make use of username and password from source

In previous patches, we have configured the wizard to automatically
ask for credentials and store them in the CollectionSource object.

Let's make use of this information, so that the user won't have to
input them again.

Created attachment 333664 [details] [review]
Add NetworkPublisher

Add a generic class that uses Avahi to handle advertising the
external IP address and the port of a connection, alongside
a description text.

We will use this to advertise SPICE connections on the local
network.

Created attachment 333665 [details] [review]
spice-display: Advertise connection on network

This patch makes a local SPICE display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

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

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

Created attachment 333666 [details] [review]
vnc-display: Add constructor machine parameter

VNC-display needs this parameter so that it can store this
information and be able, in following patches, to publish
its connection under the name of its machine. This will
also helps us check the type of the machine, so that we can
be able to publish only local displays.

Created attachment 333667 [details] [review]
vnc-display: Advertise connection on network

This patch makes a local VNC display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

Created attachment 333668 [details] [review]
vm-configurator: Handle changing listening address

We need to be able to change the address on which a SPICE
or VNC display is listening to, so that we can expose it on
the network or unexpose it.

Created attachment 333669 [details] [review]
libvirt-machine-props: Allow exposing display on net

Let the user choose whether he wants to share the SPICE or VNC
display of a local libvirt machine on the network.

Created attachment 333670 [details] [review]
Add NetworkResolver

We need a class that will help us in further patches to create
Boxes out of advertised SPICE connections.

Let's create a generic class that uses Avahi to detect and resolve
advertised connections.

Created attachment 333671 [details] [review]
wizard-source: Add WizardRemoteEntry class

In following patches we will implement a 'Remote' source page for
the wizard which will store all the remote connections discovered
on the network.

Let's add a class that represents one remote connections, both
visually and logically.

Created attachment 333672 [details] [review]
wizard-source: Add 'Remote' source page

Add a source page on which the user can choose between various
remote-connections found on the network by the NetworkResolver.

Created attachment 333673 [details] [review]
wizard: Handle 'Remote' source page

In a previous patch we added the 'Remote' source page. Let's
handle it in the wizard, to give it an appropriate behaviour.

Created attachment 333674 [details] [review]
wizard: Enable 'Preparation' and 'Setup' for remote

Currently, when creating boxes out of VNC or SPICE uris, we do
not check the connectivity of the remote connections and we do
not try to log in, thus letting the user potentially create
useless boxes.

Let's stop skipping the 'Preparation' and 'Setup' phases for
remote connections, so that we can implement them for remote
boxes in following patches.

Created attachment 333675 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise.

Created attachment 333676 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 333677 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 333678 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 333679 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 333680 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 333681 [details] [review]
wizard: Check remote connection credentials in 'Setup'

Currently, when creating a box out of a remote connection/uri
we do not check whether the user has the right credentials to
connect to it, potentially letting the user create a box to
which he has no access.

Let's check that the user has the correct credentials to access
the machine also store them in the source of the machine, for
future usage.

Created attachment 333682 [details] [review]
machine: Make use of username and password from source

In previous patches, we have configured the wizard to automatically
ask for credentials and store them in the CollectionSource object.

Let's make use of this information, so that the user won't have to
input them again.

Created attachment 333809 [details] [review]
wizard: Enable 'Preparation' and 'Setup' for remote

Currently, when creating boxes out of VNC or SPICE uris, we do
not check the connectivity of the remote connections and we do
not try to log in, thus letting the user potentially create
useless boxes.

Let's stop skipping the 'Preparation' and 'Setup' phases for
remote connections, so that we can implement them for remote
boxes in following patches.

Created attachment 333810 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise.

Created attachment 333811 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 333812 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 333813 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 333814 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 333815 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 333816 [details] [review]
wizard: Check remote connection credentials in 'Setup'

Currently, when creating a box out of a remote connection/uri
we do not check whether the user has the right credentials to
connect to it, potentially letting the user create a box to
which he has no access.

Let's check that the user has the correct credentials to access
the machine and also store them in the source of the machine, for
future usage.

Created attachment 333817 [details] [review]
machine: Make use of username and password from source

In previous patches, we have configured the wizard to automatically
ask for credentials and store them in the CollectionSource object.

Let's make use of this information, so that the user won't have to
input them again.

Created attachment 335626 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 335627 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 335628 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 335629 [details] [review]
wizard: Add RemoteConnectionHandler

Because we want to add in future patches for the wizard of remote
machines two new phases, preparation and setup, we need a way
to test remote connections.

Let's add a class that has the ability to test a remote connection
either for the preparation phase or for the setup phase and can also
handle the required wizard changes based on the result.

Created attachment 335630 [details] [review]
wizard: Set VNC source type

We need to detect the type of a vnc source for following patches
that will test SPICE and VNC remote connections in the wizard.

Created attachment 335631 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise, using the
previously introduced RemoteConnectionHandler.

Created attachment 335632 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 335633 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 335634 [details] [review]
wizard: Check remote connection credentials in 'Setup'

Currently, when creating a box out of a remote connection/uri
we do not check whether the user has the right credentials to
connect to it, potentially letting the user create a box to
which he has no access.

Let's check that the user has the correct credentials to access
the machine and also store them in the source of the machine, for
future usage.

Created attachment 335635 [details] [review]
machine: Make use of username and password from source

In previous patches, we have configured the wizard to automatically
ask for credentials and store them in the CollectionSource object.

Let's make use of this information, so that the user won't have to
input them again.

Review of attachment 331315 [details] [review]:

ack

Review of attachment 332635 [details] [review]:

ack

Review of attachment 332636 [details] [review]:

* I see that you've grouped all dep changes together. The best order is to add the dep just before using it.

* What does "handles size of ScrolledWindow much better" means? In any case, I don't think it's a good justification to require new gtk+.

Review of attachment 332637 [details] [review]:

::: src/spice-display.vala
@@ +14,3 @@
         }
     }
+    public override bool exposed_on_network { get { return uri != null && session.host != "" && !session.host.has_prefix ("127"); } }

* 120 chars limit and in this case i'd break it into multiple lines anyway.

* why not check for exact address "host != "127.0.0.1"? That way you won't need to check for empty string as well.

* The implementation is almost identical in both implementation of Display. I think it'd be better to  move VncDisplay.host to baseclass instead.

::: src/vnc-display.vala
@@ +6,3 @@
     public override string protocol { get { return "VNC"; } }
     public override string? uri { owned get { return @"vnc://$host:$port"; } }
+    public override bool exposed_on_network { get { return uri != null && host != "" && !host.has_prefix ("127"); } }

120 chars limit and in this case i'd break it into multiple lines anyway.

Review of attachment 333664 [details] [review]:

Looks good otherwise

::: src/network-publisher.vala
@@ +17,3 @@
+
+public class NetworkPublisher {
+    private string[] SERVICE_TYPES = { "_spice._tcp", "_vnc._tcp" };

this needs to be declared 'const'.

@@ +42,3 @@
+    }
+
+    public void add_service (string service_name, SERVICE_TYPE type, uint16 service_port, string? description) throws Boxes.NetworkPublisherError {

120 chars limit.

@@ +43,3 @@
+
+    public void add_service (string service_name, SERVICE_TYPE type, uint16 service_port, string? description) throws Boxes.NetworkPublisherError {
+        var final_name = service_name;

final_ seems a bit of an overkill. :) How about just 'name'?

@@ +87,3 @@
+    private string description;
+
+    public AvahiService (Avahi.Client client, string name, string type, uint16 port, string? description) throws Boxes.NetworkPublisherError {

120 chars limit again. Many would argue that 120 is already too long. :)

Review of attachment 333665 [details] [review]:

double bug URL lines.

::: src/spice-display.vala
@@ +258,3 @@
         displays.remove_all ();
+
+        if (published)

instead of checking here, best have the checks in publish/unpublish methods themselves:

if (published)
  return;

Apart from it being cleaner solution, it'll also ensure checks are always performed when these methods are called.

@@ +290,3 @@
+
+            if (published)
+                unpublish_connection ();

Seems were are unconditionally publishing/unpublishing connection on any channel being created/destroyed. I'm not even sure if new channel creation is a good place to do this but even if that's the case, we probably want to at least check if it's the main (or should it be display) channel?

@@ +311,2 @@
         case ChannelEvent.OPENED:
+            if (machine is LibvirtMachine && exposed_on_network)

not sure if it was on this bug but i clearly remember discussing that we shouldn't check for machine being LibvirtMachine. Two reasons:

* Just cause it's a LibvirtMachine, doesn't mean it's local.

* It's best to add add generic API in base classes rather than doing type checks in using code.

We have a 'is_local' property in Machine class.

@@ +435,3 @@
+        var network_publisher = NetworkPublisher.get_default ();
+        try {
+            network_publisher.add_service (machine.name, SERVICE_TYPE.SPICE, (uint16) int.parse (session.port), "Boxes SPICE connection.");

120 chars limit.

@@ +439,3 @@
+            warning (error.message);
+        }
+        published = true;

consistency is important. If you are going to have a newline before 'published = false' below, you should add it here too or viceversa.

Review of attachment 333666 [details] [review]:

The primary reason sounds good but the secondary one doesn't so log will likely need to be changed.

Review of attachment 333667 [details] [review]:

::: src/vnc-display.vala
@@ +86,3 @@
         this.machine = machine;
 
+        if (machine is LibvirtMachine && exposed_on_network)

same comments here as with spice-display.

Review of attachment 333668 [details] [review]:

"we can expose it on the network or unexpose it" -> "we can expose or unexpose it on the network", or better write "we can change it's exposure on the network".

::: src/vm-configurator.vala
@@ +528,3 @@
     }
 
+    public static void change_listen_address (Domain domain, GLib.InetAddress address) {

change_ -> set_, for consistency.

Review of attachment 333669 [details] [review]:

::: src/libvirt-machine-properties.vala
@@ +125,3 @@
+                var property_share = add_property (ref list, _("Share Display"), toggle_connection);
+
+                    toggle_connection.active = true;

code is getting a lot complicated and nested here. Can we break out into separate function(s)?

@@ +133,3 @@
+                            address = "0.0.0.0";
+                        else
+                var property_share = add_property (ref list, _("Share Display"), toggle_connection);

Why make InetAddress parse a string when you can specify in binary?

GLib.InetAddress address;

if (toggle_connection.active) {
   uint8 bytes = {0, 0, 0 , 0};
   address = new GLib.InetAddress.from_bytes (bytes);
} else {
   uint8 bytes = {127, 0, 0 , 1};
   address = new GLib.InetAddress.from_bytes (bytes);
}

Review of attachment 333670 [details] [review]:

::: src/network-resolver.vala
@@ +8,3 @@
+    private List<ServiceBrowser> service_browsers;
+
+    private string[] SERVICE_TYPES = { "_spice._tcp", "_vnc._tcp" };

* const or static keyword missing here.

* I didn't realize this before when reviewing the relevant patch but TypeNames 

  * are supposed to be in CamelCase.
  * should have a unique name (unless it's completely private to a class)

I'd name it NetworkServiceType.

* Why duplicate SERVICE_TYPES? Let's make it public in NetworkPublisher.

@@ +11,3 @@
+
+    public signal void new_service (string name, string address, int port, SERVICE_TYPE type);
+    public signal void removed_service (string name);

better and more mutually consistent names would be service_added and service_removed.

@@ +13,3 @@
+    public signal void removed_service (string name);
+
+    public NetworkResolver (SERVICE_TYPE[] service_types_values) {

I don't think service_types_values arg is needed here. We won't have a lot of service types. Let's always create browser for all known service types and let user of this class decide which ones they are interested in.

@@ +33,3 @@
+    }
+
+    private void on_service_resolved (Interface @interface,

instead of using ugly '@' keyword, I'd rather we chose a different name, e.g iface.

@@ +34,3 @@
+
+    private void on_service_resolved (Interface @interface,
+                                    Protocol protocol,

misaligned against first argument.

@@ +46,3 @@
+            if (s_type == type)
+                new_service (name, address.to_string (), (int) port, (SERVICE_TYPE) enum_value);
+            enum_value ++;

you don't need a separate enum_value. Just use for loop with a index 'i' and that's your enum.

@@ +51,3 @@
+
+    private void on_removed_service (Interface @interface,
+                                    Protocol protocol,

misaligned.

@@ +61,3 @@
+
+    private void on_new_service (Interface @interface,
+                                Protocol protocol,

misaligned

@@ +66,3 @@
+                                string domain,
+                                LookupResultFlags flags) {
+        ServiceResolver service_resolver = new ServiceResolver (Interface.UNSPEC,

perfect place for use of 'var'.

@@ +67,3 @@
+                                LookupResultFlags flags) {
+        ServiceResolver service_resolver = new ServiceResolver (Interface.UNSPEC,
+                                                Protocol.UNSPEC,

misaligned arg

Review of attachment 333671 [details] [review]:

* "we will implement a 'Remote' source page". Oh? Is this based on designer input/mockup? The URL entry page is pretty small, why not have it there?

* "one remote connections" is self-contradictory. :)

::: src/wizard-source.vala
@@ +96,3 @@
 
+[GtkTemplate (ui = "/org/gnome/Boxes/ui/wizard-remote-entry.ui")]
+public class Boxes.WizardRemoteEntry : Gtk.ListBoxRow {

'Entry' suffix gives the impression that this is for text input or some kind of input (like Gtk.Entry).

@@ +105,3 @@
+    private Gtk.Label uri_label;
+
+

_list suffix indicates it's a list (as in GLib.List e.g). parent_box would be better IMO.

@@ +106,3 @@
+
+    public WizardRemoteEntry (Gtk.ListBox parent_list, string name, string address, int port, SERVICE_TYPE type) {
+    [GtkChild]

our preferred naming scheme is reversed. :) I'd prefer type_str.

@@ +110,3 @@
+            str_type = "spice";
+        else if (type == SERVICE_TYPE.VNC)
+    [GtkChild]

why not use the genum API to get the nickname string? IIRC I pointed out example code in libvirt-glib.

@@ +112,3 @@
+            str_type = "vnc";
+        else
+    private Gtk.Label uri_label;

also a return before things get worse afterwards?

Review of attachment 332637 [details] [review]:

::: src/spice-display.vala
@@ +14,3 @@
         }
     }
+    public override bool exposed_on_network { get { return uri != null && session.host != "" && !session.host.has_prefix ("127"); } }

We still have to check for the empty string, I think. If the host string is empty, it means that the display is not exposed on the network.
I was checking for the "127" prefix because any address starting with 127 is a loopback address and can represent the host machine. I am not sure how common is for people to use loopback addresses that aren't "127.0.0.1", though.

Review of attachment 332637 [details] [review]:

::: src/spice-display.vala
@@ +14,3 @@
         }
     }
+    public override bool exposed_on_network { get { return uri != null && session.host != "" && !session.host.has_prefix ("127"); } }

* OK but it won't hurt to check for the full string since as you say, nobody ever uses any other loopback address. Besides I think elsewhere we check for full string so would be good for consistency.

* Shouldn't we also check for "localhost".

Review of attachment 332637 [details] [review]:

::: src/spice-display.vala
@@ +14,3 @@
         }
     }
+    public override bool exposed_on_network { get { return uri != null && session.host != "" && !session.host.has_prefix ("127"); } }

Actually I'm wrong about consitency. Here is what we do currently:

https://git.gnome.org/browse/gnome-boxes/tree/src/machine.vala#n60

We should probably be doing exactly that. Prerably, we should move the check into a utility function in utils-app module.

Review of attachment 333671 [details] [review]:

::: src/wizard-source.vala
@@ +110,3 @@
+            str_type = "spice";
+        else if (type == SERVICE_TYPE.VNC)
+            str_type = "vnc";

I can't recall where you have given me the example code.
Still, I am not sure a better way exists, because we don't want the service types as needed by Avahi. Here we need the actual source type.

For example, SERVICE_TYPE.VNC corresponds to the "_rfb._tcp" avahi service type, but here we simply need the "vnc" string.

Also, considering this, I should probably name the variable source_type_str.

Review of attachment 333671 [details] [review]:

::: src/wizard-source.vala
@@ +110,3 @@
+            str_type = "spice";
+        else if (type == SERVICE_TYPE.VNC)
+    [GtkChild]

Maybe I told the other student. :) Anyway here it is: http://libvirt.org/git/?p=libvirt-glib.git;a=blob;f=libvirt-gconfig/libvirt-gconfig-helpers.c;h=0314a72fd5dfcf8e47df34e7243610071f91b0ee;hb=HEAD#l248

yes, there is API to get the nickname so that SERVICE_TYPE.VNC will translate to "vnc".

"_rfb._tcp" is something you came up on your own? I'm curious about the naming policy here.

Review of attachment 333671 [details] [review]:

::: src/wizard-source.vala
@@ +110,3 @@
+            str_type = "spice";
+        else if (type == SERVICE_TYPE.VNC)
+            str_type = "vnc";

The service type generally has the following pattern: _<service protocol>._<transport_protocol> .
There are two official lists with standardized naming for service types:

http://www.dns-sd.org/serviceTypes.html (operated until 2010)

After 2010, another list has been made:
http://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xml


The service type naming is important for the compatibility with other software. I actually managed to randomly pick up a VNC remote connection at my university's library with Boxes once, so it's pretty nice. :)

Review of attachment 333671 [details] [review]:

::: src/wizard-source.vala
@@ +110,3 @@
+            str_type = "spice";
+        else if (type == SERVICE_TYPE.VNC)
+    [GtkChild]

Ah cool. Thanks for the details and good we are using standard names.

Created attachment 340028 [details] [review]
Add avahi dependencies

Boxes is going to use avahi-client and avahi-gobject to advertise
SPICE connections on the network.

Review of attachment 340028 [details] [review]:

Not modified. Patch was accepted.

Created attachment 340029 [details] [review]
Bump libvirt-glib dependency

Boxes will need to use a method for changing the address
a domain is listening to, which will be introduced in 0.2.4.

Review of attachment 340029 [details] [review]:

not modified

Review of attachment 340028 [details] [review]:

resetting status

Created attachment 340030 [details] [review]
display: Take host property from derived classes

We need the host property in the base class, in order to create
a generic implementation of the future exposed_on_network property,
which will be implemented in the next patches.

https://bugzilla.gnome.org/review?bug=694854

Created attachment 340036 [details] [review]
machine: Move uri local address check to utils-app

We will need to use this chunk of code in future patches, so
let's transform it into a utility function, in the utils-app
module.

Created attachment 340037 [details] [review]
display: Add exposed_on_network property

Add a boolean property which is true if the display
is exposed on the network and false otherwise

Created attachment 340040 [details] [review]
Add NetworkPublisher

Add a generic class that uses Avahi to handle advertising the
external IP address and the port of a connection, alongside
a description text.

We will use this to advertise SPICE connections on the local
network.

Created attachment 340041 [details] [review]
spice-display: Advertise connection on network

This patch makes a local SPICE display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

Created attachment 340042 [details] [review]
vnc-display: Add constructor machine parameter

VNC-display will need this info in order for it to be able, in
following  patches, to publish its connection under the
name of its machine.

Created attachment 340044 [details] [review]
vnc-display: Advertise connection on network

This patch makes a local VNC display which is reachable from
the network automatically publish its connection on the network.
The display automatically unpublishes itself on disconnection.

Created attachment 340045 [details] [review]
vm-configurator: Handle changing listening address

We need to be able to change the address on which a SPICE
or VNC display is listening to, so that we can change its
exposure on the network.

Created attachment 340048 [details] [review]
libvirt-machine-props: Allow exposing display on net

Let the user choose whether he wants to share the SPICE or VNC
display of a local libvirt machine on the network.

Created attachment 340049 [details] [review]
Add NetworkResolver

We need a class that will help us in further patches to create
Boxes out of advertised SPICE connections.

Let's create a generic class that uses Avahi to detect and resolve
advertised connections.

Created attachment 340052 [details] [review]
wizard-source: Add WizardConnectionRow class

In following patches we will modify the 'URL' source page, in
order to make it display all the remote connections found on
the network.

Let's add a class that represents one remote connection, both
visually and logically.

Created attachment 340053 [details] [review]
wizard: 'URL' source page -> 'Remote' source page

Let's replace the 'URL' source page with a more generic 'Remote'
source page.

In future patches, this new source page will also display
remote connections found on the network and will allow the
creation of boxes out of them.

Created attachment 340055 [details] [review]
wizard-source: Display remote connections

In previous patches we replaced the 'URL' source page to a more
generic 'Remote' source page and we have also added a class that
can resolve services for remote connections. Let's use this to
display all the remote connections available on the network on
the 'Remote' source page.

Created attachment 340056 [details] [review]
wizard: Create boxes out of resolved remote connections

In the previous patch we display on the 'Remote' source page all
the available remote connections on the network. Let's also allow
the user to create boxes out of them.

Created attachment 340057 [details] [review]
wizard: Enable 'Preparation' and 'Setup' for remote

Currently, when creating boxes out of VNC or SPICE uris, we do
not check the connectivity of the remote connections and we do
not try to log in, thus letting the user potentially create
useless boxes.

Let's stop skipping the 'Preparation' and 'Setup' phases for
remote connections, so that we can implement them for remote
boxes in following patches.

Created attachment 340058 [details] [review]
UnattendedSetupBox -> SetupBox

In following patches we will implement the wizard setup phase
for remote boxes. Let's rename the UnattendedSetupBox to a more
generic SetupBox, which will serve for both unattended and
remote cases.

Created attachment 340059 [details] [review]
setup-box: SetupBox () -> SetupBox.unattended ()

Let's rename the SetupBox's current constructor which is used for
unattended setups to a more specific SetupBox.unattended (), to
make room for the remote setup box constructor, which will come
in the next patch.

Created attachment 340060 [details] [review]
setup-box: Fit for remote case

Because we will create in following patches a wizard setup
phase for remote boxes, let's create a specific constructor
for this case.

Let's also add a spinner which indicates we are trying
to connect to a remote display and a label which tells us
that the password is wrong.

Created attachment 340061 [details] [review]
wizard: Add RemoteConnectionHandler

Because we want to add in future patches for the wizard of remote
machines two new phases, preparation and setup, we need a way
to test remote connections.

Let's add a class that has the ability to test a remote connection
either for the preparation phase or for the setup phase and can also
handle the required wizard changes based on the result.

Created attachment 340062 [details] [review]
wizard: Set VNC source type

We need to detect the type of a vnc source for following patches
that will test SPICE and VNC remote connections in the wizard.

Created attachment 340063 [details] [review]
wizard: Check for URI connectivity in 'Preparation'

Currently, when creating boxes out of VNC or SPICE uris, we do
not check if the user could actually connect to it, thus letting
the user potentially create useless boxes.

When the user tries to create a new remote box, let's first check
if the URI is reachable and show an error otherwise, using the
previously introduced RemoteConnectionHandler.

Created attachment 340064 [details] [review]
collection-source: Add 'password' property

Because of previous patches, some remote boxes need a password
at wizard's 'Setup' phase, but currently there's no way to remember
it later on.

Let's save the password and username in the CollectionSource class,
so that the user won't have to input them again and the remote
machine can make use of them.

Created attachment 340065 [details] [review]
wizard-toolbar: Add 'login' button

We need this because we are going to implement the 'Setup' page
for remote boxes, where the user will have to login to a password
protected remote connection.

Let's create a login button which, when clicked, will signify that
the user has finished inputting the required credentials.

Created attachment 340066 [details] [review]
wizard: Check remote connection credentials in 'Setup'

Currently, when creating a box out of a remote connection/uri
we do not check whether the user has the right credentials to
connect to it, potentially letting the user create a box to
which he has no access.

Let's check that the user has the correct credentials to access
the machine and also store them in the source of the machine, for
future usage.

Created attachment 340067 [details] [review]
machine: Make use of username and password from source

In previous patches, we have configured the wizard to automatically
ask for credentials and store them in the CollectionSource object.

Let's make use of this information, so that the user won't have to
input them again.

Review of attachment 340036 [details] [review]:

*util-app, not utils-app

Review of attachment 340036 [details] [review]:

sure.

Review of attachment 340037 [details] [review]:

sure.

Review of attachment 340040 [details] [review]:

I have some comments/questions about this one.

::: src/Makefile.am
@@ +166,3 @@
 	snapshot-list-row.vala 			\
 	snapshots-property.vala 		\
+	network-publisher.vala 			\

small nitpick: lets sort the files here in alphabetical order.

::: src/network-publisher.vala
@@ +58,3 @@
+        foreach (AvahiService service in services)
+            if (service.name == service_name) {
+                name = Avahi.Alternative.service_name (service_name);

It might be useful to notify the name collision anyway. I could think of a debugging scenario with duplicated services or something like that.

@@ +82,3 @@
+        case ClientState.FAILURE:
+            warning ("Avahi server failed to startup.");
+            break;

empty line before breaks.

@@ +85,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            debug ("Avahi server stopped working");

If I am not mistaken, these don't necessarily mean that the services stopped working. Is that right?
Also, can we be clearer in the last two messages?

@@ +86,3 @@
+        case ClientState.S_REGISTERING:
+            debug ("Avahi server stopped working");
+            break;

ditto.

@@ +94,3 @@
+    public string name { get; private set; }
+
+    private Avahi.EntryGroup group;

It seems to me that the NetworkPublisher should have its group, and add services into it. The way it is encapsulated in your implementation, the NetworkPublisher has a list of AvahiServices and each AvahiService has its own group. Is it expected/desirable?

@@ +142,3 @@
+    private void entry_group_state_changed (Avahi.EntryGroupState state) {
+        if (state == EntryGroupState.FAILURE)
+            warning ("Failed to publish service '%s'", name);

Are we able to identify which kind of failure happened while registering the services?

Review of attachment 340040 [details] [review]:

::: src/network-publisher.vala
@@ +85,3 @@
+        case ClientState.S_COLLISION:
+        case ClientState.S_REGISTERING:
+            debug ("Avahi server stopped working");

* I didn't manage to simulate an error scenario of this kind and unfortunately, the avahi folks aren't very responsive. Most of the information I gathered is from their official example, in which they manually remove the services after such errors and manually republish the services when the server is running again.
Because these errors are very, very rare and because I couldn't find out if the services are automatically re-published or not (Zeeshan Ali had reasons to think that the services are automatically re-published and their example of manually doing it is for demonstrative purposes), we chose not to manually remove and republish the services after a failure.

Long story short, I couldn't figure out what would happen in case this error shows up.

* Also, there doesn't seem to be any Vala API for getting the exact cause of the ClientState.FAILURE state. More details in the last comment.

Official example link:
http://www.avahi.org/doxygen/html/client-publish-service_8c-example.html

@@ +94,3 @@
+    public string name { get; private set; }
+
+    private Avahi.EntryGroup group;

Afaik, this is the intended use of Avahi's EntryGroups. The logic behind it is that certain network resources need more than one service to properly function (for example, printers). You create a group so that the fate of any of the individual service is the fate of the group. In our case, each service is an independent one. Besides, if we were to only create one group, we would have to re-publish all of the services whenever adding/removing one service.

Here is a reference:
https://lists.freedesktop.org/archives/avahi/2010-October/001946.html

@@ +142,3 @@
+    private void entry_group_state_changed (Avahi.EntryGroupState state) {
+        if (state == EntryGroupState.FAILURE)
+            warning ("Failed to publish service '%s'", name);

In their example, we notice this line:
fprintf(stderr, "Entry group failure: %s\n", avahi_strerror(avahi_client_errno(avahi_entry_group_get_client(g))));

Unfortunately, in the Vala bindings, avahi_client_errno () is not included in the API. I can't find any other way to get the last error the client received.
Also, avahi_strerror(state) wouldn't work.

Review of attachment 340040 [details] [review]:

::: src/network-publisher.vala
@@ +142,3 @@
+    private void entry_group_state_changed (Avahi.EntryGroupState state) {
+        if (state == EntryGroupState.FAILURE)
+            warning ("Failed to publish service '%s'", name);

I don't think that should be a road-block. Why not add the binding?

Review of attachment 340041 [details] [review]:

it works like a charm. lgtm

(pls push after you handle attachment 340040 [details] [review])

Review of attachment 340040 [details] [review]:

::: src/network-publisher.vala
@@ +142,3 @@
+    private void entry_group_state_changed (Avahi.EntryGroupState state) {
+        if (state == EntryGroupState.FAILURE)
+            warning ("Failed to publish service '%s'", name);

I tried to talk with the guys from avahi, but they seem to be very hard to reach/ busy. I tried to ask them if they would agree for me to add the binding, but on IRC they ignored me even after using 2 different nicknames and the same thing happened on the mailing list. 
I can not start adding stuff to avahi if I have no communication with them :(

*** Bug 764491 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-boxes/issues/10.