initial windows patch

[Peter Kümmel]

Hello Havoc,

thanks for your detailed replay!
I think I've now clearer idea how things should be done.


Havoc Pennington wrote:
> 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?

ATM the tests are build but you have to call them by hand.
But it is possible to add this functionality.

> There should not be any #ifdef DBUS_WIN outside of .c files that

OK, didn't know that this is a must.
I'll keep it in mind and change the code.

> 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.

In the patch of Ralf envp was not ifdefed and also handled by the unix
code, but when making this patch I've removed any change of the unix code
to get a clear starting point.

> 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).

Will check it also for the unix code. If it's used I could use HAVE_UNISTD_H
instead of DBUS_WIN.


>> +#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

When this is ok for the unix code?


>> +#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"

+else /* DBUS_ENABLE_VERBOSE_MODE */


> 
>> +#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.
> 

Fine, this credo simplifies things.


>> --- 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.)

I must admit that I'm ATM not very familiar with the socket and
file handling code of the win port. I've hoped that all this stuff is
mature enough so I don't have to understand it in all details, but
it seems I have to.

>>  /**
>> - * @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 ;-)

Yes, I also thought it is a little bit stupid to have win code in a *-unix
file, but sometimes the changes are so small that I didn't want to add a new
*-win file. But it is really better to have win code NOT in -unix files.


>> +/**
>> + * 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)
> 

I have to reread all you comments on this subject when I'm more familiar
with this parts of the port.

>> +  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...

Seems this is the result of c&p.


> What function in here requires malloc.h? Can we abstract it?
...
>> +        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

Then I'll add _dbus_printf_length() to sysdeps.



> 
> 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()

yes, this is important.

>> +  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.

You are right, uid=gid looks like a bug.


>> +  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.
> 

makes sense.


>>    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.
> 

Yes, this is a windows specific hack without the file iterating fails.
But maybe the port of readdir is not correct.
Seems the unix readdir does not set an error when there is no file left?
Then I have to set errno to 0 in the reader port, and remove the hack.


>> +#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.

I already had a DBUS_UNIX, but before sending the patch I've done a search
and replace over all files. :)


>> +#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
> 

Or I use #ifdef DBUS_UNIX.

>> + * 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.
> 

OK. But I think I should also add Tor and Ralf.


>> +#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"
> 

Yes, I also didn't like this very much.


>>    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
> 

Fine.


>> +#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.

This win stuff really needs a clean up.


>>    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.
> 

That's a good idea.

> 
>> -      close (fd);
>> +      DBUS_CLOSE_SOCKET (fd);
> 
> I think DBUS_CLOSE_SOCKET should probably be a function instead of a macro.

I also think that's better.

>>        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)
> 

win code:
#define DBUS_SOCKET_API_RETURNS_ERROR(n) ((n) == SOCKET_ERROR)

Checking for SOCKET_ERROR should be enough.


>> +  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.

indentation problem with tabs. What's the coding style rule for this?


>> +#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.
> 

Yes, seems better this way.

>> +  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?
> 

<stdio.h>

>> -  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?)

I've overseen this, only the format is changed.

 _dbus_string_parse_int return dbus_bool_t.



>> +#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
> 

yes, using 'handle' in the name is clearer.


> 
> Should not need this #ifdef if spawn_async is fixed.
> 
> Havoc
> 
> 

I see, much to do

Peter