[Bug 24395] Issues making telepathy-gabble 0.8.3 work on windows
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Tue Oct 13 16:00:19 CEST 2009
http://bugs.freedesktop.org/show_bug.cgi?id=24395
--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2009-10-13 07:00:14 PST ---
(From update of attachment 30158)
Ignoring lib/gibber for now, it contains most of the non-portable networking
tricks. In the 0.9 series we'll gradually use GIO (GLib 2.22) for this; if you
want to get a head start on Gabble portability, make sure you can compile GLib
2.22, and submit patches to that if you can't.
As a general matter of approach, #ifdef WIN32 is rarely the right thing to do.
The first option for portability should always be to use some functionality
that's portable, like a GLib function. Failing that, #ifdef HAVE_SOME_FUNCTION
or #ifdef HAVE_SOMETHING_H makes it clear what is going on; checking for OSs
should be a last resort, and should use the G_OS_FOO macros.
(We do need to check for OS for credentials-passing - every Unix OS implements
it differently, and so far we only know how on Linux, so in git master we use
#ifdef __linux__.)
As another general matter of approach, Telepathy has feature discovery in many
places. If you're going to make Unix sockets not work, for instance (this is
entirely reasonable, because Windows doesn't have them!) then you need to make
the feature-discovery function not return them as an option.
Similarly, GibberUnixTransport should simply not be compiled, unless G_OS_UNIX
is defined. It doesn't make sense to try to force it to compile on non-Unix
platforms.
Tubes and file transfer should be able to work even on Windows, using IPv4
sockets; we have the code for it.
>diff --git a/src/bytestream-socks5.c b/src/bytestream-socks5.c
>index f4c452b..ff1ff06 100755
>--- a/src/bytestream-socks5.c
>+++ b/src/bytestream-socks5.c
>@@ -21,23 +21,82 @@
> #include "config.h"
> #include "bytestream-socks5.h"
>
>+#ifndef WIN32
> #include <arpa/inet.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <netdb.h>
> #include <net/if.h>
> #include <netinet/in.h>
>+#endif
If these headers are non-portable, they should be checked for in configure.ac,
and guarded by #ifdef HAVE_FOO_H.
>From other bits of the code, it appears that Windows *does* have errno.h, so
that one doesn't need guarding?
>+#include <winsock2.h>
>+#include <ws2tcpip.h>
We could check for these too.
>+ #define IFNAMSIZ 16
>+#define SIZE_INTERFACES 100
>+
>+
>+struct ifmap
>+ {
>+ unsigned long int mem_start;
>+ unsigned long int mem_end;
>+ unsigned short int base_addr;
>+ unsigned char irq;
>+ unsigned char dma;
>+ unsigned char port;
>+ /* 3 bytes spare */
>+ };
>+
>+struct ifreq
>+ {
>+ char ifr_name[IFNAMSIZ]; /* Interface name */
>+ union {
>+ struct sockaddr ifr_addr;
>+ struct sockaddr ifr_dstaddr;
>+ struct sockaddr ifr_broadaddr;
>+ struct sockaddr ifr_netmask;
>+ struct sockaddr ifr_hwaddr;
>+ short ifr_flags;
>+ int ifr_ifindex;
>+ int ifr_metric;
>+ int ifr_mtu;
>+ struct ifmap ifr_map;
>+ char ifr_slave[IFNAMSIZ];
>+ char ifr_newname[IFNAMSIZ];
>+ char * ifr_data;
>+ };
>+ };
>+
>+struct ifconf
>+ {
>+ int ifc_len; /* size of buffer */
>+ union {
>+ char * ifc_buf; /* buffer address */
>+ struct ifreq * ifc_req; /* array of structures */
>+ };
>+ };
>+#define SIOCGIFCONF SIO_GET_INTERFACE_LIST
>+#define SIOCGIFFLAGS SIO_GET_INTERFACE_LIST
>+#endif
This is terrifying. Is there really no Win32 system header that can define this
structure properly?
>+
> #ifdef HAVE_GETIFADDRS
>+#ifndef WIN32
> #include <ifaddrs.h>
> #endif
>+#endif
Does Win32 define HAVE_GETIFADDRS?
>+ if ((i = WSAStartup(MAKEWORD(1,1), &info)) < 0 ) {
Shouldn't this be called from main() or something?
>+#ifndef WIN32
> /* Loop throught the interface list and get the IP address of each IF */
> for (ifr = ifc.ifc_req;
> (gchar *) ifr < (gchar *) ifc.ifc_req + ifc.ifc_len;
>@@ -1780,6 +1875,9 @@ get_local_interfaces_ips (void)
> free (ifc.ifc_req);
>
> return ips;
>+#else
>+ return NULL;
>+#endif
> }
All that work just to return NULL? :-(
>diff --git a/src/ft-manager.c b/src/ft-manager.c
>index c47f97c..eaf3bf0 100755
>--- a/src/ft-manager.c
>+++ b/src/ft-manager.c
>@@ -82,6 +87,19 @@ struct _GabbleFtManagerPrivate
> gchar *tmp_dir;
> };
>
>+#ifdef WIN32
>+char*
>+mkdtemp(char *templt) {
>+ // In windows we dont have the mkstemp or mkdtemp function
>+ // _mktemp is almost similar for creating unique file name,
>+ // but does not create the folder, and we dont know method for creating unique folder name
>+ FILE *f;
>+ _mktemp(templt);
>+ mkdir(templt);
>+ return templt;
>+}
This is not a suitable implementation of mkdtemp - you're ignoring the return
from mkdir, meaning you might be returning a directory that can be written by
someone else.
The wider issue here is that you don't actually need to implement
gabble_ft_manager_get_tmp_dir at all - the FT code uses it to make Unix
sockets, and Windows can't do Unix sockets!
>--- a/src/gabble.c
>+++ b/src/gabble.c
>+#ifndef WIN32
>+ #include <unistd.h>
>+#else
>+# ifndef STDOUT_FILENO
>+# define STDOUT_FILENO 1
>+# endif
>+# ifndef STDERR_FILENO
>+# define STDERR_FILENO 2
>+# endif
>+#endif
This should just use HAVE_UNISTD_H. We probably don't actually need unistd.h
any more here, in fact.
>@@ -84,7 +93,11 @@ stamp_log (const gchar *log_domain,
This should either just use tp_debug_timestamped_log_handler (beware that the
behaviour seems to be subtly different), or have the same fix as
telepathy-glib.
>--- a/src/jingle-content.c
>+++ b/src/jingle-content.c
>@@ -395,9 +395,15 @@ produce_senders (JingleContentSenders senders)
> }
>
>
>+#ifndef WIN32
> #define SET_BAD_REQ(txt...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_BAD_REQUEST, txt)
> #define SET_OUT_ORDER(txt) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_JINGLE_OUT_OF_ORDER, txt)
> #define SET_CONFLICT(txt...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_CONFLICT, txt)
>+#else
>+#define SET_BAD_REQ(...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_BAD_REQUEST, __VA_ARGS__)
>+#define SET_OUT_ORDER(...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_JINGLE_OUT_OF_ORDER, __VA_ARGS__)
>+#define SET_CONFLICT(...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_CONFLICT, __VA_ARGS__)
>+#endif
This should use ##__VA_ARGS__ like we do in debug.h, probably - that's
theoretically non-portable, but in practice we haven't had complaints about
debug.h, so...
>--- a/src/message-util.c
>+++ b/src/message-util.c
>@@ -37,6 +37,90 @@
> #include "namespaces.h"
> #include "util.h"
>
>+#ifdef WIN32
>+
>+// convert String to tm struct in Windows, untested
>+static int
>+_string_to_tm (char *pszTime,
>+ struct tm * tm1)
>+{
...
>+}
>+#endif
This may well be able to use g_time_val_from_iso8601, in which case we could
use it on all platforms.
>+#ifndef WIN32
> if (credentials->uid != getuid ())
> {
> DEBUG ("Wrong uid (%u). Rejecting", credentials->uid);
> goto credentials_received_cb_out;
> }
>+#else
>+ {
>+ unsigned int puid;
>+ DWORD processID = GetCurrentProcessId();
>+ puid = processID;
>+ if (credentials->uid != puid)
>+ {
>+ DEBUG ("Wrong uid (%u). Rejecting", credentials->uid);
>+ goto credentials_received_cb_out;
>+ }
>+ }
Will you ever receive credentials on Windows? If so, how?
Credentials transfer through sockets is not portable - it's a Unixism, and it
only works on Unix sockets anyway. So, Windows code can safely assume that it
can never receive correct credentials, for two reasons: (1) we can't transfer
credentials through Unix sockets on Windows, and (2) we can't even *make* Unix
sockets on Windows.
(Also, credentials->uid is a user ID, not a process ID, so comparing it to a
process ID makes no sense.)
> else if (priv->access_control == TP_SOCKET_ACCESS_CONTROL_CREDENTIALS)
This should never be reached on Windows, because you can't use
credentials-based access control unless you can transfer credentials through a
Unix socket. So, this entire elseif could safely be considered Unix-specific.
>@@ -947,9 +985,11 @@ new_connection_to_socket (GabbleTubeStream *self,
> array = g_value_get_boxed (priv->address);
> DEBUG ("Will try to connect to socket: %s", (const gchar *) array->data);
>
>+#ifndef WIN32
> transport = GIBBER_TRANSPORT (gibber_unix_transport_new ());
> gibber_unix_transport_connect (GIBBER_UNIX_TRANSPORT (transport),
> array->data, NULL);
>+#endif
If you're not running on Unix, you should already have rejected the Unix
address types by this point.
>+#ifdef WIN32
>+static gboolean
>+check_unix_params (TpSocketAddressType address_type,
>+ const GValue *address,
>+ TpSocketAccessControl access_control,
>+ const GValue *access_control_param,
>+ GError **error)
>+{
>+ return NULL;
>+}
NULL isn't a gboolean.
>--- a/src/tubes-channel.c
>+++ b/src/tubes-channel.c
>@@ -25,7 +25,16 @@
> #include <string.h>
> #include <sys/stat.h>
> #include <sys/types.h>
>+#ifndef WIN32
> #include <unistd.h>
>+#else
>+# ifndef STDOUT_FILENO
>+# define STDOUT_FILENO 1
>+# endif
>+# ifndef STDERR_FILENO
>+# define STDERR_FILENO 2
>+# endif
>+#endif
This doesn't seem to be necessary? Nothing in Tubes uses these macros.
> #include <glib/gstdio.h>
> #include <dbus/dbus-glib.h>
>@@ -1426,7 +1435,9 @@ tube_msg_close (GabbleTubesChannel *self,
>
> close_node = lm_message_node_get_child_with_namespace (msg->node, "close",
> NS_TUBES);
>+#ifndef WIN32
> g_assert (close != NULL);
>+#endif
This is a fairly obvious coding error. We're asserting that the libc function
close(3) has a non-NULL address, where we intended to assert that close_node
was non-NULL...
>diff --git a/tools/glib-ginterface-gen.py b/tools/glib-ginterface-gen.py
This has already been fixed in telepathy-glib, and the comment at the top says
"The master copy of this program is in the telepathy-glib repository - please
make any changes there". I've updated it from telepathy-glib on the branch.
--
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the telepathy-bugs
mailing list