Created attachment 34279 [details] [review]
the patch modify configure.in server/makefile.am and vino-server.c

Created attachment 34280 [details]
vino-zeroconf.c

Created attachment 34281 [details]
vino-zeroconf.h

if you are interested i can port this patch to avahi zeroconf instead of Howl,
but only if there is some interest as this patch is sitting there for almost 1 year.

Created attachment 52576 [details] [review]
Patch against vino 2.12.1 to add zeroconf support using avahi

Created attachment 52581 [details] [review]
some fix to use g_malloc instead of malloc

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

Sorry for taking so long to even look at this. Some suggestions

 - Rename the files to vino-mdns.[ch] and use mdns everywhere instead of
   zeroconf. Isn't "zeroconf" trademarked now ? MDNS more accurately
   describe what the code does, anyway

 - The rest of the code in vino/server goes to great pains to use the
   same coding style. I'd appreciate it if you could use that style
   too

 - Should be off by default in configure.in

 - I'd make the API as simple as:

     void vino_mdns_init     (void);
     void vino_mdns_shutdown (void);

   and maintain any state in static variables global to vino-mdns.[ch]

   Looks like the global variables might be:

     static AvahiClient     *mdns_client = NULL;
     static AvahiEntryGroup *mdns_group = NULL;
     static GPtrArray       *mdns_ports = NULL;

   (You'd use mdns_ports with code like:

      g_ptr_array_add (mdns_ports, GUINT_TO_POINTER (port_number));
   )

   There's no need to keep the service name as part of the global state,
   you can just create that when adding the service

 - You'd then just have 

     void vino_mdns_add_port    (int port);
     void vino_mdns_remove_port (int port);

 - The ZC_RFB define should be in vino-mdns.c as its not part of the API
   for the module

 - Ditto for all the avahi headers - they should only be included in the
   C file when we don't have the zc_vino struct

 - The use of goto in create_services() is fairly gratuitous - just return, no
   need for the variable to hold the return code even.

 - Use g_new0 (zc_vino, 1) instead of g_malloc()

 - You need to free the AvahiGLibPoll after creating the client

 - I don't see why you free everything when the client goes into DISCONNECTED
   state. Isn't there a danger that the code will attempt to free it again
   later?

 - You seem to be missing the vino-util.[ch] to add the new dprintf() module
   code

Thanks

Created attachment 52893 [details] [review]
patch the configure.in file to add avahi support (disable by default)

Created attachment 52894 [details] [review]
patch vino-server.c vino-http.c Makefile.am to add mdns support

Created attachment 52895 [details]
vino-mdns.c

Created attachment 52896 [details]
vino-mdns.h

Fixed most of the issues that you reported.

Added support to publish the built-in http server (if compiled in)

the vino-mdns API is like this:
void vino_mdns_add_service (char *type, int port);
void vino_mdns_start       (void);
void vino_mdns_stop        (void);

- vino_mdns_add_service create a hash table of service to publish (service type,
port number
- vino_mdns_start start publishing the services defined in the hash table
- vino_mdns_stop stops publishing the services and clean everything

I tryed to improve to coding style to match the one of vino.

Don't know what to do about freeing AvahiGLibPoll, and what to do in case of
DISCONNECT, i'll try to ask other avahi developpers.

thanx for your suggestions

Any thing that i should modify to make this code enter cvs?

hello, any comment on the last version of the patch?

Following is a patch supporting avahi's Api 0.6.X (the definitive one)

please review and comment, the patch is against vino CVS

Created attachment 56967 [details] [review]
Patch to add zeroconf support using avahi's api 0.6.x

Comment on attachment 56967 [details] [review]
Patch to add zeroconf support using avahi's api 0.6.x

Some comments:

>+#ifdef VINO_ENABLE_MDNS
>+  vino_mdns_add_service("_http._tcp", http_port);
>+#endif
>+

You forgot a space:
+  vino_mdns_add_service ("_http._tcp", http_port);
It happens in some other places too ;-)

>+static int create_services(AvahiClient *c, void *userdata);
>+
>+static void 
>+entry_group_callback(AvahiEntryGroup *g, AvahiEntryGroupState state, void *userdata) 
>+{
>+  char *new_name;
>+
>+  g_assert(g == mdns_group);
>+  
>+  switch(state)
>+    {
>+    case AVAHI_ENTRY_GROUP_ESTABLISHED:
>+      dprintf (MDNS, "Avahi: Service '%s' successfully established.\n", mdns_name);
>+      break;
>+    case AVAHI_ENTRY_GROUP_COLLISION:
>+
>+      new_name = avahi_alternative_service_name(mdns_name);
>+      avahi_free(mdns_name);
>+      mdns_name = new_name;
>+
>+      dprintf (MDNS, "Avahi: Service name collision, renaming service to '%s'\n", mdns_name);
>+
>+      create_services(avahi_entry_group_get_client(g), NULL);
>+      break;
>+    case AVAHI_ENTRY_GROUP_FAILURE:
>+    case AVAHI_ENTRY_GROUP_UNCOMMITED:
>+    case AVAHI_ENTRY_GROUP_REGISTERING:
>+      break;

Are the three latest cases useful? Or maybe you should add:
  default:
    g_assert_not_reached();
  after them.

>+static void 
>+client_callback(AvahiClient *c, AvahiClientState state, void * userdata) 
>+{
>+  g_assert(c);
>+
>+  switch(state)
>+    {
>+    case  AVAHI_CLIENT_S_RUNNING:
>+      create_services(c, userdata);

Isn't userdata always NULL? Do create_services() really need this second argument?

>+      break;
>+    case AVAHI_CLIENT_S_COLLISION:
>+      if (mdns_group)
>+	avahi_entry_group_reset(mdns_group);
>+      break;
>+      /* FIXME handle disconnection better */
>+    case AVAHI_CLIENT_FAILURE:
>+      dprintf (MDNS, "Connection with Avahi daemon terminated.\n");
>+      break;
>+    case AVAHI_CLIENT_CONNECTING:
>+    case AVAHI_CLIENT_S_REGISTERING:
>+      break;

Are the two latest cases useful? Or maybe you should add:
  default:
    g_assert_not_reached();
  after them.

>+}
>+
>+void 
>+vino_mdns_add_service (char *type, int port)
>+{
>+  if (mdns_services == NULL)
>+    mdns_services = g_hash_table_new (g_str_hash, g_str_equal);
>+  g_hash_table_insert (mdns_services, type, GINT_TO_POINTER (port));
>+}

It looks like calling this is useless after vino_mdns_start() has been called. Am I right? Shouldn't this be possible?

Also, I agree with Mark that a vino_mdns_remove_service() could be useful (although I'm not sure it would be used now, since I don't know vino's code).

>+void
>+vino_mdns_start ()
>+{
>+  int              error;
>+  const AvahiPoll *poll_api;
>+  AvahiGLibPoll   *glib_poll;
>+
>+  mdns_name = g_strdup_printf (_("%s's remote desktop"), g_get_user_name ());

Translators won't like this ;-) (I have no better proposition).

>+  if (!(mdns_client = avahi_client_new(poll_api, 0, client_callback, NULL, &error)))
>+    {
>+      dprintf (MDNS, "Avahi daemon may not be running.\n");
>+      vino_mdns_stop();
>+    }  

Don't use &error if you won't use it (and free it).

Also, it shouldn't be possible to call vino_mdns_start() when it's already started, or it shouldn't do anything.

>+void
>+vino_mdns_stop (void)
>+{
>+  g_free(mdns_name);
>+  if (mdns_group)
>+    avahi_entry_group_free(mdns_group);

You can add:
  mdns_group = NULL;

>+  if (mdns_client)
>+    avahi_client_free(mdns_client);

Same for mdns_client.

>+  if (mdns_services)
>+    g_hash_table_destroy(mdns_services);

Same for mdns_services.

Created attachment 56980 [details] [review]
New patch implementing vincent recommendations

I implemented your recommendations, i just didn't change the stuff about vino_mdns_remove_service as i think it's not usefull yet, as there is no reason to  publish only vnc and not http.

In my opinion it's publish vnc+http or don't publish anything, in this use case vino_mdns_start/stop() is enought.

Created attachment 56982 [details] [review]
new version of the patch

this part in vino-server.c:
#ifdef VINO_ENABLE_MDNS
  vino_mdns_stop ();
#endif /* VINO_ENABLE_MDNS */

could be move before #ifdef VINO_ENABLE_HTTP_SERVER
so the service would be unpublished before the http server is stopped.

Thanks guys - I've finally got around to looking at this again, re-worked it a bit and committed.

I've only given it basic testing, so if you could bang on it a bit more, that would be great.

2006-01-13  Mark McLoughlin  <mark@skynet.ie>

        Add support for publishing over mDNS. Based on patch from very
        patient Sebastien Estienne <sebastien.estienne@gmail.com> in
        bug #159874

        * configure.in: add --enable-avahi

        * server/Makefile.am: add vino-mdns.[ch] and build against
        avahi if enabled

        * server/vino-mdns.[ch]:
        (vino_mdns_add_service),
        (vino_mdns_start),
        (vino_mdns_stop): add internal publishing API.

        * server/vino-main.c: (main): start and stop the mDNS support
        before and after entering the mainloop.

        * server/vino-server.c: (vino_server_init_from_screen): advertise
        the rfb service

        * server/vino-http.c: (vino_http_create_listening_socket): advertise
        the http service

        * server/vino-util.[ch]: (vino_setup_debug_flags): add mdns
        debugging flag