Re: glib 2.3.3 and Windows

["J. Ali Harlow" <ali juiblex co uk>]

On 2004.02.28 12:34 Hans Breuer wrote:
> At 07:44 26.02.04, J. Ali Harlow wrote:
> > On 2004.02.25 23:56 Hans Breuer wrote:
> > > At 00:14 25.02.04, J. Ali Harlow wrote:
>
> [...]
>
> > > > My situation is that I have chosen to base the latest application I'm 
> > > > writing on glib 2.3.2 on the basis that 2.4.0 would be released in 
> > > > plenty of time before I was ready to release my own stable version. 
> > > > This now seems in jeopardy.
> > > Are you about to release your version very soon, do you think some 
> > > small
> > > build glitches are jeopardy - or did I miss some breakage ?
> >
> > I attached my win32 implementation of g_child_watch to bug 50296, which
> > seemed a sensible place since that was the origin of Matthias's changes.
> >
> > I think my implementation is much better than yours :-)
>
> Me too ;-) Just commited your implementation.

Thanks.

> > Specifically it doesn't get confused if the child exits with code 259
> > (STILL_ACTIVE) and it doesn't need yet another thread. There aren't
> > many cases where it's actually easier to do something under win32 than
> > UNIX, but we might as well take advantage of the few that do exist.
> >
> > I also opened bug 135386 and attached a fix to add the missing exports
> > to glib.def. I see you've added at least some of these, so this can
> > probably be closed.
> >
> > > > Given this, I would like to look at solving these problems if noone 
> > > > else more qualified is available to look at these issues this week.
> > > I'm certainly not qualified for configure magic, but ...
> > >
> > > > I attach what I have so far - a patch to glibconfig.h to at least 
> > > > allow the build to progress to gmain.c (I don't know if including 
> > > > windows.h from glibconfig.h is a good idea yet, so I haven't yet 
> > > > submitted this to bugzilla).
> > > ... including windows.h in glibconfig.h would cause huge breakage in 
> > > application code due to massive namespace clashes (at least in Dia).
> >
> > Yes, I can't say I'm completely surprised. I see you've just used int
> > under msc, which I'd be happy with but Tor seemed to be trying to avoid
> > this (apparantly because handles won't fit into ints under win64).
>
> changed to typedef void* GPid, though I don't think win64 isn't an issue
> at all yet.

Quite so. There are many existing assumptions that HANDLEs fit into ints.
I assume Tor was just suggesting we didn't add another one.

> I've left out your configure changes cause IIRC
> glibconfig.h.win32.in and config.h.win32.in have allways been a merge
> of the output of configure (for the mingw32 build) and some manually
> tweaking required for the msvc build, which does not use the autotools
> at all.

That's fine. I'll bug Tor to validate the change to configure.in and
commit it once a final decision on HANDLE/ProcessID is reached...

> > Personally I advocate making GPid hold ProcessIDs anyway for reasons I
> > go into in my comments to 50296. I'd appreciate your views.
>
> If this isn't solved with the patch, please explain further ;)

My patch implemented g_child_watch using HANDLES (same as yours) since
that's what Tor had suggested even though I believe it would be better
to use ProcessIDs. If we were to use ProcessIDs, then GPid would be a
DWORD (presumably typed as an unsigned long). g_child_watch_source_new()
would look something like this:

GSource *
g_child_watch_source_new (GPid pid)
{
  GSource *source = g_source_new (&g_child_watch_funcs,
      sizeof (GChildWatchSource));
  GChildWatchSource *child_watch_source = (GChildWatchSource *)source;
             #ifdef G_OS_WIN32
  child_watch_source->poll.fd = 
(int)OpenProcess(PROCESS_QUERY_INFORMATION |
      SYNCHRONIZE, FALSE, pid);
  if (!child_watch_source->poll.fd)
    ...
  child_watch_source->poll.events = G_IO_IN;
      g_source_add_poll (source, &child_watch_source->poll);
#else /* G_OS_WIN32 */
  g_child_watch_source_init ();
#endif /* G_OS_WIN32 */

  child_watch_source->pid = pid;
                return source;
}

g_child_watch_dispatch() would need to pass the handle (rather than the 
pid)
to GetExitCodeProcess and close the handle just before returning.

The caller would probably need to use GetProcessId() to get the ProcessID
from the HANDLE.

I admit that this is somewhat less efficient than using HANDLEs directly
(since in most cases the caller is more likely to have a HANDLE than a
ProcessID), but it does make it quite clear who is responsible for closing
the handle and when which I otherwise see as a possible source of bugs
(the caller closes it before the callback is invoked causing errors from
the poll function) or of leaks (the caller never closes the handle).

There are, of course, other alternatives:

- We could document that g_child_watch_source_new and friends take 
ownership
of the handle passed in. This would mean that we'd just need to add a
CloseHandle at the end of dispose.

- We could duplicate the handle in g_child_watch_source_new.

In any case, staying with handles means that we need to document what the
user should do and this needs to be fixed before 2.4.0 is released and
preferably before the API freeze since it's really part of the API.

Cheers,

--
J. Ali Harlow
ali juiblex co uk