bug#19688: [patch] add support for emacs daemon on Windows

[Mark Laws <mdl@60hz.org>]

On Mon, Jan 26, 2015 at 3:00 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> [Please keep the bug address on the CC list, so that this whole
> discussion gets archived.]

Oops, blindly hit reply last time without noticing where it was going
to--sorry about that.

>> Date: Mon, 26 Jan 2015 08:16:38 +0900
>> From: Mark Laws <mdl@60hz.org>
>>
>> >> +#define W32_EMACS_SERVER_GUID "{0B8E5DCB-D7CF-4423-A9F1-2F6927F0D318}"
>> >
>> > Where did this GUID come from?
>> >
>> I generated it myself.
>
> Is that safe?  Do we care whether this GUID is globally unique?  Why
> exactly do we need it to begin with?

It should be safe. On UNIX, Emacs uses a pipe to tell emacsclient when
it's done initializing. On Windows, since we don't have fork, the
easiest options are either a named event object[1] or specifying that
the child process inherit the event handle in CreateProcess. The
former is simpler, so I went with it. We could call it
"EmacsDaemonEvent" or something instead; it doesn't really matter as
long as it's a name nothing else is likely to use.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682396%28v=vs.85%29.aspx

>> > Why do we need anything beyond the event handle?  Can't it serve
>> > double duty as a flag as well?  We could use INVALID_HANDLE_VALUE
>> > and/or NULL as distinct values with specific meanings.
>>
>> I've reworked it to use only w32_daemon_event. One minor issue: to
>> avoid including any Windows-related headers in lisp.h, the extern
>> declaration of w32_daemon_event there is void* instead of HANDLE. I
>> doubt the type of HANDLE will ever change, so it shouldn't matter
>> much.
>
> That should be fine, but there's no need to have the handle in lisp.h,
> either.  We could put it on w32.h, for example, which is already
> included by emacs.c and other files.  If w32.h doesn't fit the bill,
> please try to find some other w32*.h which does.

IS_DAEMON is defined in lisp.h, so it seems to make the most sense to
put it there rather than having a different IS_DAEMON for Windows in
another place. We'd have to #include a w32 header just for IS_DAEMON
in a lot more files if we move it to one of them, too.

>> >> +#ifndef WINDOWSNT
>> >>    /* Make sure IS_DAEMON starts up as false.  */
>> >>    daemon_pipe[1] = 0;
>> >> +#endif
>> >
>> > You should do a similar initialization on WINDOWSNT, to avoid using
>> > the value that was initialized when Emacs was dumped.
>>
>> I'm not sure I understand. Do you mean for w32_daemon_event? If so, it
>> should already always be zero when Emacs starts up.
>
> It might not be zero.  Recall that Emacs is run during the build
> process, when it executes some code, and then "dumps itself", which
> involves writing portions of its memory to a disk file that thereafter
> becomes the emacs.exe you run normally.  If, for some reason,
> w32_daemon_event is initialized in this first instance, it will not be
> zero in the dumped image.
>
> So we always re-initialize such variables explicitly, instead of
> relying on the linker initializations.  You can see that in the
> sources, and that is also the reason why the Posix code initializes
> daemon_pipe[1].

OK, fixed.

>> > Also, the call to daemon_check_preconditions should be outside of the
>> > #ifdef, as it is used on all platforms.
>>
>> daemon_check_preconditions already gets called for both WINDOWSNT and
>> !WINDOWSNT (see the #else). If we move it outside the #ifdef to
>> eliminate having to mention it for both cases, then "nfd" and "err"
>> will also have to be outside the ifdef in order to be compatible with
>> strict pre-C99 compilers that only accept variable declarations at the
>> beginning of blocks.
>
> C99 compliant compiler is a prerequisite in the development version of
> Emacs, so this problem doesn't exist.

Fixed.

I'll attach another patch when we figure out what to do about the
remaining issues.

-- 
|v\ /\ |\ |< |_ /\ \^| //