Created attachment 8094 [details] [review]
patch file

This patch renames public API which we can't do regaurdless if there are
applications using it or not.

what's a "handle" in this context? I thought you were using sockets on Windows?

This patch results in a bunch of things like:

 fd = dbus_watch_get_handle

which doesn't make sense. 

Remember we have no cross-platform idea of a file handle. We have a cross-
platform idea of a socket, which is either a UNIX socket or winsock socket. 
API for this should be named get_socket, etc. get_socket would not return a 
non-socket even on UNIX, it would e.g. return -1 for a UNIX file handle.

API that is unix-specific and returns any file descriptor, not just a socket, 
should be called get_unix_fd and should not work on Windows at all (always 
return -1). See dbus_connection_get_unix_fd() (I think)

dbus_watch_get_fd is really mis-named, which was known when we went to 1.0 but 
we didn't want to impose breakage. I think it should really be documented 
as "this should be considered to be dbus_watch_get_unix_fd() and always 
returns -1 on Windows"

I would then add a dbus_watch_get_socket() and use that when possible.

If Windows ever has non-socket transports, we might need to add 
dbus_watch_get_whatever() to return whatever the non-socket transport uses.

If you really are using Windows handles and not sockets, then I would add a 
dbus_watch_get_windows_handle() that always returns -1 on unix. Be sure the 
data type it returns is guaranteed >= sizeof(HANDLE) though.

(In reply to comment #2)
> This patch renames public API which we can't do regaurdless if there are
> applications using it or not.

The patch 1. renames get_fd to get_handle 
and 2. then adds get_fd again, with the same
functionality on the Unix side.
get_handle is only used internally.

Effectively the API doesn't change on the Unix side.

(In reply to comment #3)
> what's a "handle" in this context? I thought you were using sockets on Windows?
> 
> This patch results in a bunch of things like:
> 
>  fd = dbus_watch_get_handle

Yes, this looks really inconsistent.

> which doesn't make sense. 
> 
> Remember we have no cross-platform idea of a file handle. We have a cross-
> platform idea of a socket, which is either a UNIX socket or winsock socket. 
> API for this should be named get_socket, etc. get_socket would not return a 
> non-socket even on UNIX, it would e.g. return -1 for a UNIX file handle.

get_handle is not mentioned to be part of the public API, anly get_fd, as before.

> API that is unix-specific and returns any file descriptor, not just a socket, 
> should be called get_unix_fd and should not work on Windows at all (always 
> return -1). See dbus_connection_get_unix_fd() (I think)

Currently it is also possible to get a file descriptor on windows,
so it is not necessary to introduce _unix_.

> dbus_watch_get_fd is really mis-named, which was known when we went to 1.0 but 
> we didn't want to impose breakage. I think it should really be documented 
> as "this should be considered to be dbus_watch_get_unix_fd() and always 
> returns -1 on Windows"
> 

The cleanest way is to have no fd/HANDLE in any public function,
only DBusSockets, DBusFiles,...

> I would then add a dbus_watch_get_socket() and use that when possible.
> 
> If Windows ever has non-socket transports, we might need to add 
> dbus_watch_get_whatever() to return whatever the non-socket transport uses.
> 
> If you really are using Windows handles and not sockets, then I would add a 

the handle is not a 'Windows HANDLE' it is our encrypted fd.

> dbus_watch_get_windows_handle() that always returns -1 on unix. Be sure the 
> data type it returns is guaranteed >= sizeof(HANDLE) though.

A other way to fix the problem with QtDBus is to introduce a new function 
and to patch Qt.

> Effectively the API doesn't change on the Unix side.

Better:
The public API doesn't change at all.

The patch fixes a bug on Windows, because get_fd 
currently doesn't return a fd but a *-win.cpp internal 
handle value, totally useless for the client.

Because of this the declaration of get_handle is guarded:

 int          dbus_watch_get_fd      (DBusWatch        *watch);
+#if defined (DBUS_COMPILATION)
+int          dbus_watch_get_handle  (DBusWatch        *watch);
+#endif

Internal API should start with an underscore and be in an -internal.h header. 
Internal DBusWatch API should not be used in the bus daemon or outside of the 
dbus/dbus/ directory, though. If you want to use this API in the bus daemon it 
will need to be public.

I don't like the name get_handle() ... if we use the word handle it should 
mean a Windows native handle. I thought we killed off the need for "fake" file 
descriptors when we made the change to have sockets cross-platform? When would 
the watch be on anything other than a socket?

> Currently it is also possible to get a file descriptor on windows,
> so it is not necessary to introduce _unix_.

The Windows public API imo should have a) sockets and b) Windows native HANDLE 
only, and always be clear about which one is returned.

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

2007-06-18  Havoc Pennington  <hp@redhat.com>

        * dbus/dbus-watch.c (dbus_watch_get_socket)
        (dbus_watch_get_unix_fd): new API to match DBusConnection
        (dbus_watch_get_fd): deprecate this

        Throughout: just s/dbus_watch_get_fd/dbus_watch_get_socket/g for
        now since all the transports use sockets anyway