On 10/29/2022 11:14 PM, Eli Zaretskii wrote:
>> Date: Sat, 29 Oct 2022 14:33:42 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Attached is a patch to do this. Note that I named the new argument
>> "noframe" because that matches the existing code in server.el (see
>> 'server-delete-client'). It's a bit of a misnomer though, and maybe
>> "keep-frames" would be better...
> 
> Hmm... it doesn't look very elegant to add to server-start an extra
> argument whose purpose is to affect server-delete-client.  Can we do
> this in some more elegant way?  For example, how about making most of
> the code in server-start an internal function with an additional
> argument, and then have server-start and server-force-stop call that
> internal function?  At the very least the new argument to server-start
> shouldn't be advertised, I think, since it's entirely for internal use
> by us.

Thanks for taking a look. I had hesitated to make any big changes to 
this code since it doesn't have regression tests, but I think the 
attached patch should be more elegant while retaining the flow of the 
code as much as possible. Do you know of a good way to write regression 
tests for this code? I couldn't find any existing tests that start/stop 
Emacs processes in a way that I could use for testing this. It seems 
like we'd need to be able to start a new Emacs process and then be able 
to send arbitrary key combinations to it to drive it...

Back to the patch itself though: one small functional change I made was 
that I slightly changed how 'server-ensure-safe-dir' is called in 
'server-start'. (I only mention this because this code looks to be 
security-related, so I think it should have extra attention.)

It used to check the 'server-dir', defined as:

   (if server-use-tcp server-auth-dir server-socket-dir)

Now, it checks the directory of the server *file*. The file is defined as:

   (expand-file-name server-name server-dir)

This could be different if 'server-name' (a defcustom) contains a slash. 
I think the new code is actually safer, since it checks the proper 
directory even if a user customized 'server-name' to have a slash or 
'..' or whatever. Still, I thought this change deserved a mention so 
that I don't inadvertently open a security hole.