initial windows patch
Havoc Pennington
hp at redhat.com
Wed Jul 5 19:13:55 PDT 2006
Hi,
A macro question before getting into the patch comments: is it possible
to run the "make check" with your windows build setup? Meaning, are we
getting good test coverage on Windows?
Peter Kümmel wrote:
> diff -u -B -b -r1.41 activation.c
> --- bus/activation.c 22 Nov 2005 20:37:00 -0000 1.41
> +++ bus/activation.c 3 Jul 2006 12:56:20 -0000
> @@ -33,7 +33,11 @@
> #include <dbus/dbus-shell.h>
> #include <dbus/dbus-spawn.h>
> #include <dbus/dbus-timeout.h>
> +#ifdef DBUS_WIN
> +#include <dbus/dbus-dirent-win.h>
> +#else
There should not be any #ifdef DBUS_WIN outside of .c files that
implement a cross-platform abstraction. Usually the #ifdef would be in
dbus-sysdeps.c for example, though some more complex abstractions should
maybe have their own dbus-dirent-win32.c/dbus-dirent-unix.c for example.
> _dbus_verbose ("Spawning %s ...\n", argv[0]);
> if (!_dbus_spawn_async_with_babysitter (&pending_activation->babysitter, argv,
> +#ifdef DBUS_WIN
> + envp,
> +#endif
The #ifdef here should be somewhere else; I think there was a previous
discussion about how dbus spawn/babysitter doesn't work at all or make
sense on Windows, which I expect is true; if so then there should be a
cross-platform abstraction at a higher level.
But if it does make sense, there should at least be an abstraction that
addresses environment variables vs. however things work on Windows.
> +#ifndef SIGHUP
> +#define SIGHUP 1
> +#endif
This at least needs a comment about when SIGHUP would not be defined.
> +#ifndef DBUS_WIN
> #include <unistd.h>
> +#endif
If unistd is not used in the file, this should just be deleted; if it
is, then there's more thinking to do (depends on how it's used).
> +#ifdef DBUS_WIN
> + const char *p = _dbus_getenv ("DBUS_VERBOSE");
> + verbose = p != NULL && *p == '1';
> +#else
> verbose = _dbus_getenv ("DBUS_VERBOSE") != NULL;
> +#endif
Why not just add the *p == 1 to unix also and drop the #ifdef
> +#ifdef HAVE_GNUC_VARARGS
> +# define _dbus_verbose(format, rest...) _dbus_verbose_real("[%s %d] " format, __FUNCTION__,__LINE__, ## rest)
> +#else
> # define _dbus_verbose _dbus_verbose_real
> +#endif
> # define _dbus_verbose_reset _dbus_verbose_reset_real
> +
> #else
> +
> # ifdef HAVE_ISO_VARARGS
> # define _dbus_verbose(...)
This logic looks messed up - "if else end else"
> +#ifdef DBUS_WIN
> +_DBUS_DECLARE_GLOBAL_LOCK (win32_fds);
> +_DBUS_DECLARE_GLOBAL_LOCK (sid_atom_cache);
> +#endif
> _DBUS_DECLARE_GLOBAL_LOCK (list);
> _DBUS_DECLARE_GLOBAL_LOCK (connection_slots);
> _DBUS_DECLARE_GLOBAL_LOCK (pending_call_slots);
> @@ -281,7 +291,11 @@
> _DBUS_DECLARE_GLOBAL_LOCK (system_users);
> _DBUS_DECLARE_GLOBAL_LOCK (message_cache);
> _DBUS_DECLARE_GLOBAL_LOCK (shared_connections);
> +#ifdef DBUS_WIN
> +#define _DBUS_N_GLOBAL_LOCKS (13)
> +#else
> #define _DBUS_N_GLOBAL_LOCKS (11)
> +#endif
I'd just drop the #ifdef here and always have the two windows locks,
saving a little memory is not worth having platform-specific bugs such
as wrong N_GLOBAL_LOCKS on only one platform.
> --- dbus/dbus-mainloop.c 15 Jan 2005 07:15:38 -0000 1.18
> +++ dbus/dbus-mainloop.c 3 Jul 2006 12:56:30 -0000
> @@ -609,7 +609,7 @@
>
> flags = dbus_watch_get_flags (wcb->watch);
>
> - fds[n_fds].fd = dbus_watch_get_fd (wcb->watch);
> + fds[n_fds].fd = _dbus_re_encapsulate_socket(dbus_watch_get_fd (wcb->watch));
How do you know the watch is on a socket? (I don't know all the kinds of
handle on Windows; if it's just files on the filesystem and sockets, I
guess files on the filesystem isn't ever going to happen. But if there
are also pipes and the like, we would need to worry about this.)
> /**
> - * @defgroup DBusServerUnix DBusServer implementations for UNIX
> + * @defgroup DBusServerUnix DBusServer implementations for UNIX and Winsock
If we do it this way, the file should not be named dbus-server-unix.c ;-)
> +/**
> + * Creates a new server listening on the given Windows named pipe.
...
> +DBusServer*
> +_dbus_server_new_for_domain_socket (const char *path,
> + dbus_bool_t abstract,
> + DBusError *error)
If this uses a Windows named pipe and not a UNIX domain socket, the
function should be called _dbus_server_new_for_named_pipe() and the
address format should be different. i.e. instead of
DBUS_SESSION_BUS_ADDRESS being something like
unix:path=/tmp/foo, DBUS_SESSION_BUS_ADDRESS should be maybe
win_named_pipe:name=whatever
The code that chooses to call dbus_server_new_for_domain_socket based on
the unix: address should then choose to call
_dbus_server_new_for_named_pipe based on the win_named_pipe: address.
Feel free to name the address something better than win_named_pipe (and
if we have existing address types with "-" or "_" should be consistent)
> + if ((abstract &&
> + !_dbus_string_append (&address, "unix:abstract=")) ||
> + (!abstract &&
> + !_dbus_string_append (&address, "unix:path=")) ||
> + !_dbus_string_append (&address, path))
...
> + listen_fd = _dbus_listen_unix_socket (path, abstract, error);
> +
I think this is inside the win32-only function? Nothing with _unix in
the name should be inside #ifdef DBUS_WIN, or implemented on Windows -
that's just too confusing for me...
> + _dbus_fd_set_close_on_exec (listen_fd);
Does windows have close on exec and what does it mean on Windows?
> + server = _dbus_server_new_for_fd (listen_fd, &address);
This is a windows socket handle, not an fd...
> + unix_server = (DBusServerUnix*) server;
> + unix_server->socket_name = path_copy;
Some renaming to do here, or the more unix-named things I see in this
file the more I think you should just do dbus-server-win32.c instead of
#ifdef inside dbus-server-unix.c
> /**
> - * Creates a new server listening on the given hostname and port.
> - * If the hostname is NULL, listens on localhost.
> + * Creates a new server listening on the given hostname and port. If
> + * the hostname is NULL, listens on localhost. If the hostname is an
> + * empty string, listens on any local host address.
Is there a change in the patch for this, or is it just standard behavior
for listen()? Does dbus ever rely on this behavior - maybe empty string
should be an error?
> dbus_bool_t _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p,
> char **argv,
> +#ifdef DBUS_WIN
> + char **envp,
> +#endif
Already mentioned
> +#ifdef DBUS_WIN
> +#include <malloc.h>
> +#endif
What function in here requires malloc.h? Can we abstract it?
> /**
> * @defgroup DBusString string class
> * @ingroup DBusInternals
> @@ -1193,7 +1197,6 @@
> va_list args)
> {
> int len;
> - char c;
> va_list args_copy;
>
> DBUS_STRING_PREAMBLE (str);
> @@ -1201,7 +1204,34 @@
> DBUS_VA_COPY (args_copy, args);
>
> /* Measure the message length without terminating nul */
> +#ifndef DBUS_WIN
> + {
> + char c;
> len = vsnprintf (&c, 1, format, args);
> + }
> +#else
> + /* MSVCRT's vsnprintf semantics are a bit different */
> + /* The C library source in the Platform SDK indicates that this
> + * would work, but alas, it doesn't. At least not on Windows
> + * 2000. Presumably those sources correspond to the C library on
> + * some newer or even future Windows version.
> + *
> + len = _vsnprintf (NULL, _DBUS_INT_MAX, format, args);
> + */
> + {
> + char p[1024];
> + len = vsnprintf (p, sizeof(p)-1, format, args);
> + if (len == -1) // try again
> + {
> + char *p;
> + p = malloc (strlen(format)*3);
> + len = vsnprintf (p, sizeof(p)-1, format, args);
> + free(p);
If the malloc.h include was for this, just use _dbus_malloc instead
here. Also, I think it would be better to add a _dbus_printf_length() or
something to dbus-sysdeps
> +#else /* DBUS_WIN */
> +
> + dbus_set_error_const (error, DBUS_ERROR_FAILED,
> + "Not implemented: _dbus_become_daemon\n");
> + return FALSE;
> +
> +#endif /* DBUS_WIN */
Windows maybe doesn't have the special setup for daemons? If so, it
would make more sense to me for this function to do nothing (but
succeed) on Windows, than to have it fail and require people to remove
the "become daemon" setting from config files or whatever.
If there is stuff to do for daemon mode on Windows, then what you have
here is fine but I'd also add a doxygen @todo
> +#ifndef DBUS_WIN
> +
> /* setgroups() only works if we are a privileged process,
> * so we don't return error on failure; the only possible
> * failure is that we don't have perms to do it.
> @@ -277,6 +294,15 @@
> }
>
> return TRUE;
> +
> +#else /* DBUS_WIN */
> +
> + dbus_set_error_const (error, DBUS_ERROR_FAILED,
> + "Not implemented: _dbus_change_identity\n");
> +
> + return FALSE;
> +
> +#endif /* DBUS_WIN */
> }
Similarly I'd add an @todo if this is a FIXME, and if it's just not
something we want to do on Windows, have some sort of abstraction.
The point both here and for become_daemon() is that we don't want to
treat "did not work" and "does not make sense on platform" in the same
way. Imagine you're the caller of the function; how would you call the
function such that it works properly on both Windows and UNIX. We need
an answer to that for all functions.
> /** Installs a UNIX signal handler
> @@ -288,6 +314,7 @@
> _dbus_set_signal_handler (int sig,
> DBusSignalHandler handler)
> {
It'd be worth adding unix to the name of this function perhaps. Also, if
this is why we needed the #define SIGHUP in the other file, adding a
_DBUS_SIGHUP in dbus-sysdeps.h might be nice.
> + out4:
> + dbus_free (console_user_sid);
> + out3:
> + CloseWindowStation (winsta);
> + out2:
> + dbus_free (user_sid);
> + out0:
> + dbus_free (wusername);
console_user_sid and user_sid (don't know about wusername) were
allocated by Windows, not by dbus_malloc; so they need to be freed with
regular free() - dbus_malloc() may not be the same as malloc()
> + statbuf->uid = _dbus_win_sid_to_uid_t (owner_sid);
> + statbuf->gid = _dbus_win_sid_to_uid_t (group_sid);
This ends up looking like a bug since you're assigning uid to gid -
maybe just adding win_sid_to_gid_t would be a nice fix.
> + statbuf->atime =
> + (((dbus_int64_t) wfad.ftLastAccessTime.dwHighDateTime << 32) +
> + wfad.ftLastAccessTime.dwLowDateTime) / 10000000 - DBUS_INT64_CONSTANT (116444736000000000);
> +
A macro for this would be nice since it's done 3 times here, and I think
also elsewhere.
> ent = readdir (iter->d);
> if (ent == NULL)
> {
> +#ifdef DBUS_WIN
> + if (errno != 0 && iter->d->finished != 1)
> +#else
> if (errno != 0)
> +#endif
If this is a windows-specific check, it at least needs a comment on why
Windows is different. If it's not a windows-specific fix (or at least is
harmless on unix), then no #ifdef - but maybe still a comment.
> dbus_set_error (error,
> _dbus_error_from_errno (errno),
> "%s", _dbus_strerror (errno));
> @@ -548,6 +740,7 @@
> dbus_free (iter);
> }
>
> +#ifndef DBUS_WIN
> static dbus_bool_t
> fill_user_info_from_group (struct group *g,
> DBusGroupInfo *info,
> @@ -568,6 +761,7 @@
>
> return TRUE;
> }
> +#endif /* DBUS_WIN */
should be "#endif /* !DBUS_WIN */" right?
I think introducing a DBUS_UNIX and preferring ifdef to ifndef might be
a lot clearer though.
> + info->groupname = dbus_malloc (strlen (domain) + 1 + strlen (name) + 1);
> +
> + strcpy (info->groupname, domain);
> + strcat (info->groupname, "\\");
> + strcat (info->groupname, name);
Should use DBusString rather than strcpy/strcat.
> +#ifndef DBUS_WIN
> +
...
> while (sep > 0 && _dbus_string_get_byte (filename, sep - 1) == '/')
> --sep;
>
> @@ -733,6 +992,52 @@
> else
> return _dbus_string_copy_len (filename, 0, sep - 0,
> dirname, _dbus_string_get_length (dirname));
> +#else
Would swap the code blocks to use #ifdef instead of #ifndef
> +#ifndef DBUS_WIN
> _dbus_string_init_const (&str, "0xff");
> if (!_dbus_string_parse_double (&str,
> 0, &val, &pos))
> @@ -848,12 +1174,23 @@
> _dbus_warn ("_dbus_string_parse_double of \"0xff\" returned wrong position %d", pos);
> exit (1);
> }
> +#endif
This doesn't look like it should be #ifndef DBUS_WIN, at least not
without a comment or @todo or FIXME.
Something I suggested earlier was using a DBUS_WIN_FIXME or
DBUS_WIN_TEMPORARY or something to bracket temporarily-disabled stuff,
vs. stuff that's more long term.
> + * Copyright (C) 2005 Novell, Inc.
Should btw add your own 2006 copyright to files where you wrote more
than a line or two of changes.
> +#ifndef DBUS_WIN
> +#define _dbus_decapsulate_quick(i) (i)
> +#define DBUS_SOCKET_IS_INVALID(s) ((s) < 0)
> +#define DBUS_SOCKET_API_RETURNS_ERROR(n) ((n) < 0)
> +#define DBUS_SOCKET_SET_ERRNO() /* empty */
> +#define DBUS_CLOSE_SOCKET(s) close(s)
> +#endif
> +
Not sure at this point in reading the patch how to fix, but it's a
little confusing here that the Windows definitions aren't alongside
these; I'd consider reorganizing a little bit to have the two
definitions alongside each other. Failing that, at least a comment like
"Windows definitions are in foo.h"
> if (s && *s)
> _dbus_print_backtrace ();
> #endif
> +#if defined (DBUS_WIN) && defined (__GNUC__)
> + if (IsDebuggerPresent ())
> + __asm__ __volatile__ ("int $03");
> +#endif
This needs a comment about what the heck it is ;-)
> +#ifdef DBUS_WIN
> + putenv_value = malloc (len + 2);
> +#else
> putenv_value = malloc (len + 1);
> +#endif
Just avoid the #ifdef and have +2 on both platforms, with a comment that
2 is needed on Windows for whatever reason (what is the reason?)
> DBusString *buffer,
> int count)
> {
> +#ifdef DBUS_WIN
> + return _dbus_read_win (fd, buffer, count);
> +#else
> int bytes_read;
> int start;
> char *data;
> @@ -237,6 +269,7 @@
>
> return bytes_read;
> }
> +#endif
> }
Can this only be used on sockets on windows? If so have we audited that
it never gets used on files in cross-platform code?
>
> /**
> @@ -255,6 +288,9 @@
> int start,
> int len)
> {
> +#ifdef DBUS_WIN
> + return _dbus_write_win (fd, buffer, start, len);
> +#else
(similarly to read)
> +#ifdef DBUS_WIN
> + return _dbus_write_two_win(fd, buffer1, start1, len1, buffer2, start2, len2);
> +#else
(and here too)
> #define _DBUS_MAX_SUN_PATH_LENGTH 99
> @@ -383,6 +426,10 @@
> * given path. The connection fd is returned, and is set up as
> * nonblocking.
> *
> + * On Windows there are no UNIX domain sockets. Instead, connects to a
> + * localhost-bound TCP socket, whose port number is stored in a file
> + * at the given path.
> + *
> * Uses abstract sockets instead of filesystem-linked sockets if
> * requested (it's possible only on Linux; see "man 7 unix" on Linux).
> * On non-Linux abstract socket usage always fails.
> @@ -397,6 +444,10 @@
> dbus_bool_t abstract,
> DBusError *error)
> {
> +#ifdef DBUS_WIN
> + return _dbus_connect_unix_socket_win(path, abstract, error);
Come on, this function name is just absurd ;-)
But the point isn't the name, it's the approach. We should not "emulate"
UNIX domain sockets, we should use something sensible on Windows. dbus
is already set up for multiple kinds of transport (tcp, unix concrete,
unix abstract, pipe). Just add a windows transport, and keep the
unix-specific operations unix-specific.
> +#ifdef DBUS_WIN
> + return _dbus_listen_unix_socket_win(path, abstract,error);
> +#else
> +
(same thing)
> fd = socket (AF_INET, SOCK_STREAM, 0);
>
> - if (fd < 0)
> + if (DBUS_SOCKET_IS_INVALID (fd))
> {
> + DBUS_SOCKET_SET_ERRNO ();
> dbus_set_error (error,
> _dbus_error_from_errno (errno),
> "Failed to create socket: %s",
Why jump through hoops to make "errno" work on Windows; just provide a
_dbus_get_last_error() that uses errno on unix and GetLastError() or
whatever on windows.
> if (host == NULL)
> + {
> host = "localhost";
> +#ifdef DBUS_WIN
> + ina.s_addr = htonl (INADDR_LOOPBACK);
> + haddr = &ina;
> +#endif
> + }
This deserves a comment on why localhost won't work.
> he = gethostbyname (host);
> if (he == NULL)
> {
> + DBUS_SOCKET_SET_ERRNO ();
> dbus_set_error (error,
> _dbus_error_from_errno (errno),
> "Failed to lookup hostname: %s",
> host);
Same errno comment.
> - close (fd);
> + DBUS_CLOSE_SOCKET (fd);
I think DBUS_CLOSE_SOCKET should probably be a function instead of a macro.
> return -1;
> }
>
> @@ -674,22 +747,26 @@
> addr.sin_family = AF_INET;
> addr.sin_port = htons (port);
>
> - if (connect (fd, (struct sockaddr*) &addr, sizeof (addr)) < 0)
> + if (DBUS_SOCKET_API_RETURNS_ERROR
> + (connect (fd, (struct sockaddr*) &addr, sizeof (addr)) < 0))
This is very confusing - why is there a "<0" _and_ the
DBUS_SOCKET_API_RETURNS_ERROR?
(Remember I don't have the windows definition of
DBUS_SOCKET_API_RETURNS_ERROR to look at)
> +#ifdef DBUS_WIN
> + /* FIXME: for the session bus credentials shouldn't matter (?), but
> + * for the system bus they are presumably essential. A rough outline
> + * of a way to implement the credential transfer would be this:
You simply don't need to implement credential transfer if you don't want
to. The auth mechanism is extensible; the current mechanisms are
EXTERNAL (credential transfer via the OS) and DBUS_COOKIE_SHA1 (a
file-based approach). You could implement EXTERNAL on Windows, or you
could just not offer EXTERNAL on Windows and have a
DBUS_SOME_WINDOWS_MECHANISM instead.
> _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>
> +#ifndef DBUS_WIN
> directory = _dbus_string_get_const_data (dir);
>
> if (stat (directory, &sb) < 0)
> @@ -1255,7 +1411,7 @@
> "%s directory is not private to the user", directory);
> return FALSE;
> }
> -
> +#endif
> return TRUE;
> }
Does this need a FIXME, comment, ... ? not sure what's going on.
> @@ -1339,6 +1495,10 @@
> ascii_strtod (const char *nptr,
> char **endptr)
> {
> + /* FIXME: The Win32 C library's strtod() doesn't handle hex.
> + * Presumably many Unixes don't either.
> + */
> +
Should be able to try strtol() also or something like that.
> + if (uid != DBUS_UID_UNSET)
> + {
> + if (!fill_win_user_info_from_uid (uid, info, error)) {
> + _dbus_verbose("%s after fill_win_user_info_from_uid\n",__FUNCTION__);
> + return FALSE;
> + }
> + }
braces here are wonky - should be on their own line, indented, etc.
> +#ifndef DBUS_WIN
> /* The POSIX spec certainly doesn't promise this, but
> * we need these assertions to fail as soon as we're wrong about
> * it so we can do the porting fixups
> @@ -1793,10 +1987,11 @@
> _dbus_assert (sizeof (pid_t) <= sizeof (credentials->pid));
> _dbus_assert (sizeof (uid_t) <= sizeof (credentials->uid));
> _dbus_assert (sizeof (gid_t) <= sizeof (credentials->gid));
> +#endif
>
> - credentials->pid = getpid ();
> - credentials->uid = getuid ();
> - credentials->gid = getgid ();
> + credentials->pid = _dbus_getpid ();
> + credentials->uid = _dbus_getuid ();
> + credentials->gid = _dbus_getgid ();
> }
If you added _dbus_getpid() etc., then those should return dbus types
not pid_t. That means the assertions above should move into
_dbus_getpid() etc., where the rest of the #ifdef would be, and the
above #ifdef can be removed.
> /**
> @@ -1830,7 +2025,11 @@
> unsigned long
> _dbus_getpid (void)
> {
> +#ifndef DBUS_WIN
> return getpid ();
> +#else
> + return GetCurrentProcessId ();
> +#endif
> }
So the assertions should be here; assert that sizeof(pid_t) <=
sizeof(unsigned long), or on Windows that whatever GetCurrentProcessId()
returns is also <= unsigned long.
>
> /** Gets our UID
> @@ -1839,7 +2038,11 @@
> dbus_uid_t
> _dbus_getuid (void)
> {
> +#ifdef DBUS_WIN
> + return _dbus_getuid_win ();
> +#else
> return getuid ();
> +#endif
> }
>
And here assert the corresponding thing.
> + const int encapsulated_fd = _dbus_encapsulate_fd (fd);
>
"const" in this context is not used anywhere else in dbus, it's more of
a C++ thing.
> while (total < (int) sb.st_size)
> {
> - bytes_read = _dbus_read (fd, str,
> + bytes_read = _dbus_read (encapsulated_fd, str,
> sb.st_size - total);
Here you're using _dbus_read on a file, right, and iirc _dbus_read had
some code that looked (but apparently was not) socket-specific. Should
be sure socket or file specific functions are clearly and correctly named.
> + if (_dbus_close (fd, NULL) < 0)
A "dbus native" _dbus_close returning a dbus error might return
dbus_bool_t instead of int ... looking at the patch below it seems like
it does in fact, I'm not looking at the actual code though so it may be
patch confusion.
> + if (
> +#ifdef DBUS_WIN
> + (unlink (filename_c) == -1 && errno != ENOENT) ||
> +#endif
> + rename (tmp_filename_c, filename_c) < 0)
I thought unlink() was a unix function and errno didn't work on windows?
or is it in the std C library?
In any case, I don't understand this code; unlink and rename don't do
the same thing. What happens to tmp_filename_c, doesn't it need
deleting? And copying to filename_c?
> +#ifndef DBUS_WIN
> dir_ends_in_slash = '/' == _dbus_string_get_byte (dir,
> _dbus_string_get_length (dir) - 1);
>
> @@ -2458,6 +2701,25 @@
> if (!_dbus_string_append_byte (dir, '/'))
> return FALSE;
> }
> +#else
> + dir_ends_in_slash =
> + ('/' == _dbus_string_get_byte (dir, _dbus_string_get_length (dir) - 1) ||
> + '\\' == _dbus_string_get_byte (dir, _dbus_string_get_length (dir) - 1));
> +
> + file_starts_with_slash =
> + ('/' == _dbus_string_get_byte (next_component, 0) ||
> + '\\' == _dbus_string_get_byte (next_component, 0));
> +
> + if (dir_ends_in_slash && file_starts_with_slash)
> + {
> + _dbus_string_shorten (dir, 1);
> + }
> + else if (!(dir_ends_in_slash || file_starts_with_slash))
> + {
> + if (!_dbus_string_append_byte (dir, '\\'))
> + return FALSE;
> + }
> +#endif
>
Is there some handling of "C:" type stuff needed here? e.g. could you
append "C:" and "foo" to get "C:\foo" ?
> +#ifndef DBUS_WIN
> /* note, urandom on linux will fall back to pseudorandom */
> fd = open ("/dev/urandom", O_RDONLY);
> +#endif
> +
> if (fd < 0)
> return pseudorandom_generate_random_bytes (str, n_bytes);
>
Looks like this needs a @todo / FIXME
> #if !defined (DBUS_DISABLE_ASSERT) || defined(DBUS_BUILD_TESTS)
> @@ -2893,8 +3183,8 @@
> _dbus_parse_uid (const DBusString *uid_str,
> dbus_uid_t *uid)
> {
> + dbus_uid_t val;
> int end;
> - long val;
>
> if (_dbus_string_get_length (uid_str) == 0)
> {
> @@ -2904,8 +3194,7 @@
>
> val = -1;
> end = 0;
> - if (!_dbus_string_parse_int (uid_str, 0, &val,
> - &end))
> + if (!_dbus_string_parse_int (uid_str, 0, &val, &end))
> {
This change doesn't seem Windows related and looks like it would create
a compiler warning (doesn't parse_int return a signed value?)
> +#elif defined (DBUS_WIN)
> + return _dbus_full_duplex_pipe_win (fd1, fd2, blocking, error);
Does Windows have/need full duplex pipes in the unix sense? Are they
interchangeable with sockets on Windows for read/poll/close/etc.?
> +#define _dbus_encapsulate_socket(socket) (socket)
> +#define _dbus_re_encapsulate_socket(socket) (socket)
> +#define _dbus_encapsulate_fd(fd) (fd)
> +#define _dbus_re_encapsulate_fd(fd) (fd)
> +#define _dbus_decapsulate(fd) (fd)
These should probably do something on UNIX so UNIX developers don't
break things. e.g. they could use the high bit of the fd to flag sockets
vs. not sockets and fail if the two are misused in a way that would fail
on Windows.
I'd like to name these something a bit more descriptive as well; I'm not
sure exactly what they do on Windows so not sure what to suggest, but
for example maybe _dbus_handle_from_socket, _dbus_handle_from_file,
dbus_handle_to_socket, dbus_handle_to_file
To get maximum value from this encapsulation, we would not make these
macros/functions public; they'd be used only in a single .c file, which
would also export operations such as read, poll, etc. on "dbus handles"
Then there's no "cheating"
Also, _dbus_decapsulate needs to have _fd in the name to match the
others (or otherwise match whatever we rename the others to)
> +#ifndef DBUS_WIN
> run_data_test ("spawn", specific_test, _dbus_spawn_test, test_data_dir);
> +#endif
Needs a fixme or @todo or perhaps move the #ifdef into _dbus_spawn_test
so it does nothing on Windows.
> Index: dbus/dbus-transport-unix.c
> ===================================================================
> RCS file: /cvs/dbus/dbus/dbus/dbus-transport-unix.c,v
> retrieving revision 1.49
> diff -u -B -b -r1.49 dbus-transport-unix.c
> --- dbus/dbus-transport-unix.c 30 May 2006 15:34:10 -0000 1.49
> +++ dbus/dbus-transport-unix.c 3 Jul 2006 12:56:40 -0000
> @@ -26,6 +26,9 @@
> #include "dbus-transport-unix.h"
> #include "dbus-transport-protected.h"
> #include "dbus-watch.h"
> +#ifdef DBUS_WIN
> +#include "dbus-sockets-win.h"
> +#endif
Should not be a win.h include in a file called -unix
(though for both this and the server-unix.c file, renaming it locally
will be a pain since it'd lose the patch history by confusing cvs, so
you could rename just after committing or something.)
> int
> dbus_watch_get_fd (DBusWatch *watch)
> {
> - return watch->fd;
> + if (watch->fd == -1)
> + return -1;
> + else
> + return _dbus_decapsulate (watch->fd);
> }
_dbus_decapsulate should just handle -1 maybe?
> +#ifdef DBUS_WIN
> + if (!_dbus_spawn_async_with_babysitter (NULL, argv_copy, setup_func, NULL, NULL, &error))
> +#else
> if (!_dbus_spawn_async_with_babysitter (NULL, argv_copy, setup_func, NULL, &error))
> +#endif
Should not need this #ifdef if spawn_async is fixed.
Havoc
More information about the dbus
mailing list