Re: [PATCH] A myriad of interrelated daemon-mode startup bugs

[Nix]

On 13 Nov 2012, Stefan Monnier spake thusly:

>> [Stefan Cc:ed because part of this is fixing lexical-binding fallout,
>>  and as a way of saying thank you for fixing a bug of mine: here's a
>>  fix for a bug of yours! :) ]
>
> Actually, it's got nothing to do with lexical-binding, but is due to
> a bug of mine in gv-define-simpler-setter which was fixed yesterday.

Oh. you mean I wasted all that time? ;}

>> --- lisp/desktop.el.orig     2012-11-13 16:36:38.833375966 +0000
>> +++ lisp/desktop.el  2012-11-13 16:38:18.634889091 +0000
>> @@ -627,6 +627,7 @@
>>                     (and
>>                      (or (memq desktop-save '(ask ask-if-new))
>>                          (and exists (eq desktop-save 'ask-if-exists)))
>> +                    (and (not noninteractive))
>>                      (y-or-n-p "Save desktop? ")))))
>
> No need for `and' here, but otherwise, that looks OK.  Tho maybe a better
> fix is to make it so y-or-n-p just returns nil when there's no live
> terminal.

Good idea. (Whatever part of 'noninteractive' 'no live terminal'
equates to.)

>> -      (cl-letf (((default-file-modes) ?\700)) (make-directory dir t))
>> +      (make-directory dir t ?\700)
>
> The cl-letf ends up doing the exact same thing, via dynamic scoping
> (note how it's not a simple `let' and it doesn't bind a variable but
> a "place").

Ah! Mind you, there's not much hint in the docs for cl-letf that it's
dynamically bound...

> I'm not completely opposed to adding a "modes" argument to
> make-directory, but I'm not sure it's worth the trouble.

It's a potential security hole to make directories with the wrong mode,
so it seems like a good idea. (Sure, you can bind `default-file-modes',
but could you really call the code above particularly clear?)

>> -      (server-start)
>> +      (condition-case err
>> +      (server-start)
>> +    (error (message (error-message-string err))))
>>        (if server-process
>>        (daemon-initialized)
>>      (if (stringp dn)
>
> I'm not sure I understand enough of what could/should happen.  E.g. how
> does the above compare to using:
>
>     (unwind-protect
>         (server-start)
>       (if server-process
>           (daemon-initialized)
>         (if (stringp dn)
>         ...

That works, but it throws away the error message.  If `server-start'
raised an error, you want to show it to the user before you die.

>> +    ;; We must pretend to be noninteractive at this point to tell
>> +    ;; kill-emacs-hook functions that read input if interactive
>> +    ;; not to do so, since reads from stdin are shared with the
>> +    ;; parent.
>> +    (let ((noninteractive t))
>> +        (kill-emacs 1)))))
>
> Actually, `noninteractive' can sometimes interact with the user via
> stdin/stdout.

OK, so that ugly kludge is presumably unnecessary? Phew.

>> -    /* End of file in -batch run causes exit here.  */
>> -    if (noninteractive)
>> +    /* End of file in -batch run or before the daemon is
>> +       initialized causes exit here.  */
>> +    if ((noninteractive) ||
>> +        (IS_DAEMON && !daemon_initialized))
>>        Fkill_emacs (Qt);
>
> How 'bout having a Lisp-exported variable that controls whether to call
> kill-emacs here?  Then server.el could set this var (after successfully
> starting the daemon) to prevent exit.

Instead of daemon-initialized? The worry there is that server-start can
be called from places *other* than daemonization, and it seems like a
layering violation to have it saying 'yeah, you can exit now' if it was
called from somewhere else. The relevant thing here isn't really 'a
server is started': it's 'we have disconnected from our parent and
gone daemon, so having a command loop is acceptable because it won't
mislead users into thinking Emacs has hung'.

> Also, maybe the right way to solve this is to think harder about what
> noninteractive means and how to split it into several variables.
> E.g. one meaning is "no input events", another is "I/O is via
> stdin/stdout", and another we need is "there's no I/O available right
> now".

That's exactly what I was thinking while I got repeatedly confused over
just what 'noninteractive' meant right now. At the moment, it seems to
be used to mean whichever of the above is most convenient at the time :)

-- 
NULL && (void)