bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Thierry Volpiatto]

Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the emacs-pretest-bug@gnu.org mailing list.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

When killing emacs --daemon or emacs brutally with killall for example,
when reloading emacs or emacs --daemon, emacs ask if we want to load
desktop file or not (pid is already in use...).
I think emacs should check if the PID that is in his lock file is always
in use.
i made a quick hack for that:
That go after line 967 in desktop.el in function desktop-read.

,----
| (eq 0 (call-process-shell-command (format "ps -u %s | grep emacs | grep %d" (getenv "USER") owner)))
`----

I can make a patch if you want, just tell me.

This bug is not really a bug if we consider using emacs normally:
we have just to answer yes or no.
But that particularly anoying with new feature emacs --daemon.
If this one is started as a service it can hang the system and booting
the machine come impossible until file .emacs.desktop.lock is removed.
(Gentoo provide an init script to start emacs --daemon as service with openrc)

Thank you.
Thierry

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
/usr/share/emacs/23.0.60/etc/DEBUG for instructions.


In GNU Emacs 23.0.60.1 (i686-pc-linux-gnu, GTK+ Version 2.14.5)
 of 2008-12-02 on tux
Windowing system distributor `The X.Org Foundation', version 11.0.10300000
configured using `configure  '--prefix=/usr' '--host=i686-pc-linux-gnu' '--mandir=/usr/share/man' '--infodir=/usr/share/info' '--datadir=/usr/share' '--sysconfdir=/etc' '--localstatedir=/var/lib' '--program-suffix=-emacs-23' '--infodir=/usr/share/info/emacs-23' '--with-sound' '--with-x' '--without-toolkit-scroll-bars' '--with-gif' '--with-jpeg' '--with-png' '--with-rsvg' '--with-tiff' '--with-xpm' '--with-freetype' '--with-xft' '--without-libotf' '--without-m17n-flt' '--with-x-toolkit=gtk' '--without-hesiod' '--without-kerberos' '--without-kerberos5' '--with-gpm' '--with-dbus' '--build=i686-pc-linux-gnu' 'build_alias=i686-pc-linux-gnu' 'host_alias=i686-pc-linux-gnu' 'CFLAGS=-march=i686 -pipe -O2' 'LDFLAGS=-Wl,-O1''

Important settings:
  value of $LC_ALL: fr_FR.UTF-8
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: fr_FR.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: ERC

Minor modes in effect:
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-networks-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-netsplit-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  icomplete-mode: t
  icicle-mode: t
  delete-selection-mode: t
  minibuffer-depth-indicate-mode: t
  auto-image-file-mode: t
  partial-completion-mode: t
  shell-dirtrack-mode: t
  display-wireless-mode: t
  display-battery-mode: t
  display-time-mode: t
  diff-auto-refine-mode: t
  recentf-mode: t
  savehist-mode: t
  desktop-save-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
m e ! ) <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <left> <left> <left> <left> <left> <left> <left> 
<left> <left> <left> <left> <left> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> C-e 
<return> A n y SPC <backspace> w a y SPC i SPC w i 
l l SPC s u b m i t SPC a SPC b u g SPC a l s o . <return> 
T h a n k s . <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <right> <right> 
<right> <backspace> <left> <left> <left> <left> <left> 
g m a n e . <right> <right> <right> <right> <right> 
<right> <left> . <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<tab> <tab> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <tab> <tab> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <down> 
<down> <down> <down> <down> <down> C-SPC C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-x r s a C-g C-g C-c C-c y 
q q n <return> <return> q <return> <return> q q y <switch-frame> 
h k B s t : <backspace> <left> <left> <left> <left> 
<left> H i ! SPC <right> <right> <right> <right> <right> 
<return> M-x e m a c s - s u b m i <tab> <backspace> 
<backspace> <backspace> <backspace> <backspace> <tab> 
b u g <tab> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
b u g <S-iso-lefttab> <down> <down> <down> <down> <down> 
C-g M-x r e p o r t - e <tab> b u g <return>

Recent messages:
Scoring...done
Generating summary...done
No more unread articles
No more unread newsgroups
Are you sure you want to quit reading news? (y or n) 
Wrote /home/thierry/.newsrc
Saving /home/thierry/.newsrc.eld...
Wrote /home/thierry/.newsrc.eld
Saving /home/thierry/.newsrc.eld...done
Computing completion candidates... [5 times]

-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Juanma Barranquero]

On Tue, Dec 2, 2008 at 22:43, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:

> | (eq 0 (call-process-shell-command (format "ps -u %s | grep emacs | grep %d" (getenv "USER") owner)))

In Emacs 23.X I think you could use `system-process-attributes'
instead of running an external ps process.

But one way or the other, what if the desktop lock file resides in a
non-local drive?

  Juanma

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Thierry Volpiatto]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On Tue, Dec 2, 2008 at 22:43, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>
>> | (eq 0 (call-process-shell-command (format "ps -u %s | grep emacs | grep %d" (getenv "USER") owner)))
>
> In Emacs 23.X I think you could use `system-process-attributes'
> instead of running an external ps process.

Yes, here is the code that can be used in desktop.el:

,----
| (and (system-process-attributes owner)
|      (string-match "emacs"
|                    (cdr (assq 'comm
|                               (system-process-attributes owner)))))
`----

> But one way or the other, what if the desktop lock file resides in a
> non-local drive?

If emacs process number <owner> doesn't exist, the "else" block is used.
  
-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Juanma Barranquero]

On Wed, Dec 3, 2008 at 07:36, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:

> If emacs process number <owner> doesn't exist, the "else" block is used.

Please explain: if the desktop file that Emacs finds out is locked by
a process running in another computer, what do you want the local
Emacs to do?

  Juanma

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Thierry Volpiatto]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On Wed, Dec 3, 2008 at 07:36, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>
>> If emacs process number <owner> doesn't exist, the "else" block is used.
>
> Please explain: if the desktop file that Emacs finds out is locked by
> a process running in another computer, what do you want the local
> Emacs to do?

I am sorry but I don't understand why emacs would find a process on
another computer. In which case ?

I just want emacs to check if the process that is in the lock file is
own by another emacs process.

* If no emacs process with this pid is found ==> load desktop

* If an emacs process with this pid is found ==> ask (default) if we load
  desktop or not

Actually when the .emacs.desktop.lock is found, emacs lock desktop and
ask if we want to use the desktop file, even if the pid that is in the
file is no more in use by an emacs session.

Emacs should do that only if user start a second emacs session, in this
case the lock file will do his work and tell user this desktop is
already in use.

To reproduce, start emacs with (desktop-save 1) in your .emacs
open some buffers, then kill emacs with a killall or similar.
Now no emacs process exist right?
start emacs again, it will tell you an emacs process is using desktop:
thats wrong.

I hope it's clear,
thanks.

-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Juanma Barranquero]

On Wed, Dec 3, 2008 at 09:47, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:

> I am sorry but I don't understand why emacs would find a process on
> another computer.

No, I'm saying that Emacs *won't* find a process in another computer.

> In which case ?

The user can customize desktop.el so the desktop file (and desktop
file lock) are anywhere, even in a shared directory in a remote disk.

> I just want emacs to check if the process that is in the lock file is
> own by another emacs process.

Yes, I understand. And I'm saying that in the presence of distributed
filesystems, you cannot guarantee that a desktop lock file in a local
disk is owned by a local process, or that a local Emacs is saving the
desktop locally.

> * If no emacs process with this pid is found ==> load desktop
>
> * If an emacs process with this pid is found ==> ask (default) if we load
>  desktop or not
>
> Actually when the .emacs.desktop.lock is found, emacs lock desktop and
> ask if we want to use the desktop file, even if the pid that is in the
> file is no more in use by an emacs session.

I share the sentiment. In fact, I do exactly that in my .emacs.

But the point of the desktop locking mechanism is protecting the user;
automatically overwriting the desktop is against that idea.

  Juanma

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Thierry Volpiatto]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On Wed, Dec 3, 2008 at 09:47, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>
>> I am sorry but I don't understand why emacs would find a process on
>> another computer.
>
> No, I'm saying that Emacs *won't* find a process in another computer.
>
>> In which case ?
>
> The user can customize desktop.el so the desktop file (and desktop
> file lock) are anywhere, even in a shared directory in a remote disk.
Ok i understand now.
In this case, may be a variable can be set:

* Use a remote desktop ==> use the .emacs.desktop.lock like the actual
  code

* Use a local desktop ==> in this case check if pid exist.

(i think local desktop is the most common case)


>> I just want emacs to check if the process that is in the lock file is
>> own by another emacs process.
>
> Yes, I understand. And I'm saying that in the presence of distributed
> filesystems, you cannot guarantee that a desktop lock file in a local
> disk is owned by a local process, or that a local Emacs is saving the
> desktop locally.
>
>> * If no emacs process with this pid is found ==> load desktop
>>
>> * If an emacs process with this pid is found ==> ask (default) if we load
>>  desktop or not
>>
>> Actually when the .emacs.desktop.lock is found, emacs lock desktop and
>> ask if we want to use the desktop file, even if the pid that is in the
>> file is no more in use by an emacs session.
>
> I share the sentiment. In fact, I do exactly that in my .emacs.
>
> But the point of the desktop locking mechanism is protecting the user;
> automatically overwriting the desktop is against that idea.
>
>   Juanma
>

-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Juanma Barranquero]

On Wed, Dec 3, 2008 at 11:08, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:

> Ok i understand now.
> In this case, may be a variable can be set:
>
> * Use a remote desktop ==> use the .emacs.desktop.lock like the actual
>  code
>
> * Use a local desktop ==> in this case check if pid exist.

If you have a shared dir in your local disk, it could be used by a
remote Emacs; but you know nothing about that Emacs (well, *you*
perhaps do know, but the local Emacs does not).

> (i think local desktop is the most common case)

I agree, but it should work sensibly (and safely) in the uncommon cases too :-)

             Juanma

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Stefan Monnier]

> I am sorry but I don't understand why emacs would find a process on
> another computer. In which case ?

E.g. when the user's HOME directory is shared by NFS?


        Stefan

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Ulrich Mueller]

>>>>> On Wed, 3 Dec 2008, Juanma Barranquero wrote:

> Please explain: if the desktop file that Emacs finds out is locked
> by a process running in another computer, what do you want the local
> Emacs to do?

So in addition to the process id, the lock file should also contain
the host name, so that the most common usage case can be handled.

And instead of a lock file with new syntax, couldn't desktop.el use a
symlink, identical to what is done for file locking? (Or even use
functions "lock-buffer" etc.?)

Ulrich

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Juanma Barranquero]

On Wed, Dec 3, 2008 at 10:14, Ulrich Mueller <ulm@gentoo.org> wrote:

> So in addition to the process id, the lock file should also contain
> the host name, so that the most common usage case can be handled.

That's an idea worth pursuing.

> And instead of a lock file with new syntax, couldn't desktop.el use a
> symlink, identical to what is done for file locking? (Or even use
> functions "lock-buffer" etc.?)

Only if you want to remove functionality for users of Emacs on Windows.

  Juanma

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Stefan Monnier]

> (Gentoo provide an init script to start emacs --daemon as service with openrc)

That sounds "brain-dead" since Emacs is not useful to the system, only
to its users.


        Stefan

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Ulrich Mueller]

>>>>> On Tue, 02 Dec 2008, Stefan Monnier wrote:

>> (Gentoo provide an init script to start emacs --daemon as service
>> with openrc)

> That sounds "brain-dead" since Emacs is not useful to the system,
> only to its users.

Of course the rc script has a multiplexed configuration, so that the
started Emacs daemons will run under the users' id. Otherwise it
wouldn't make much sense.

Ulrich

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Thierry Volpiatto]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> (Gentoo provide an init script to start emacs --daemon as service with openrc)
>
> That sounds "brain-dead" since Emacs is not useful to the system, only
> to its users.

The init script is started in a by user process with a symlink, in the
same manner as net.eth1 is a symlink of net.lo.
So the init script is started as /etc/init.d/emacs.<USER> and the
process is started as <USER>.
That's a concept, it's interesting, and can interest other users.
-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]

tags 1474 + patch
thanks

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> When killing emacs --daemon or emacs brutally with killall for example,
> when reloading emacs or emacs --daemon, emacs ask if we want to load
> desktop file or not (pid is already in use...).
> I think emacs should check if the PID that is in his lock file is always
> in use.

I agree with the proposal, and have written up a suggested patch.
This would work as follows (taken from my NEWS entry, see the
suggested documentation for more):

    ** Emacs Sessions (Desktop)
    
    +++
    *** New option to load if locking Emacs not running locally.
    The option 'desktop-load-locked-desktop' can now be set to value
    'check', which means to load the desktop only if the locking Emacs
    process is not running on the local machine.  See the "(emacs) Saving
    Emacs Sessions" node in the Emacs manual for details.

The concerns with this proposal was that this breaks if Emacs is
running on a remote machine.  The user could have the lock file in a
remoted directory (e.g. on an NFS mount).  I have therefore documented
in the manual and the doc string that this value should be avoided
under such circumstances.

Another idea suggested in this thread was to change the lock file to
also include (system-name).  That could be done, if it's deemed to be
better, but has the drawback that a lock file from a recent Emacs
would not be recognized by an old Emacs.  (The relevant code reads the
whole buffer.)  A possible work-around is to add the backwards
incompatible system name in a comment instead.  Maybe that is much
more desirable than simply documenting the limitation; it would be
good to hear other opinions on that.

Any comments or suggestions?

Best regards,
Stefan Kangas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Load-desktop-without-prompting-if-process-is-dead.patch --]
[-- Type: text/x-diff, Size: 7824 bytes --]

From 04c2d26df4f73be675cc9ea6aa2ce10a474ecd18 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 24 Jan 2020 05:12:20 +0100
Subject: [PATCH] Load desktop without prompting if process is dead

* lisp/desktop.el (desktop-load-locked-desktop): Add new value 'check'
to load desktop file without prompting if locking Emacs process does
not exist on the local machine.  (Bug#1474)
(desktop-read): Extract function from here...
(desktop--load-locked-desktop-p): ...to here.  New function handles
the semantics of 'desktop-load-locked-desktop', including above new
value 'check'.
(desktop--emacs-pid-running-p): New function.

* test/lisp/desktop-tests.el: New file with tests for the above.

* doc/emacs/misc.texi (Saving Emacs Sessions): Document the new
'check' value.
* etc/NEWS: Announce the change.
---
 doc/emacs/misc.texi        |  7 +++++-
 etc/NEWS                   |  9 +++++++
 lisp/desktop.el            | 48 +++++++++++++++++++++++++++---------
 test/lisp/desktop-tests.el | 50 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 12 deletions(-)
 create mode 100644 test/lisp/desktop-tests.el

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 6b95b12a84..bedbfb7abe 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -2653,7 +2653,12 @@ Saving Emacs Sessions
 can avoid the question by customizing the variable
 @code{desktop-load-locked-desktop} to either @code{nil}, which means
 never load the desktop in this case, or @code{t}, which means load the
-desktop without asking.
+desktop without asking.  Finally, the @code{check} value means to load
+the file if the Emacs process that has locked the desktop is not
+running on the local machine.  This should not be used in
+circumstances where the locking Emacs might still be running on
+another machine.  This could be the case in multi-user environments
+where your home directory is mounted remotely using NFS or similar.
 
 @cindex desktop restore in daemon mode
   When Emacs starts in daemon mode, it cannot ask you any questions,
diff --git a/etc/NEWS b/etc/NEWS
index 11ef31b2c8..de39912e90 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -90,6 +90,15 @@ supplied error message.
 *** New connection method "media", which allows accessing media devices
 like cell phones, tablets or cameras.
 
+** Emacs Sessions (Desktop)
+
++++
+*** New option to load if locking Emacs not running locally.
+The option 'desktop-load-locked-desktop' can now be set to value
+'check', which means to load the desktop only if the locking Emacs
+process is not running on the local machine.  See the "(emacs) Saving
+Emacs Sessions" node in the Emacs manual for details.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/desktop.el b/lisp/desktop.el
index 9538bb4a34..27f6c80531 100644
--- a/lisp/desktop.el
+++ b/lisp/desktop.el
@@ -230,16 +230,25 @@ desktop-auto-save-timeout
 (defcustom desktop-load-locked-desktop 'ask
   "Specifies whether the desktop should be loaded if locked.
 Possible values are:
-   t    -- load anyway.
-   nil  -- don't load.
-   ask  -- ask the user.
-If the value is nil, or `ask' and the user chooses not to load the desktop,
-the normal hook `desktop-not-loaded-hook' is run."
+   t      -- load anyway.
+   nil    -- don't load.
+   ask    -- ask the user.
+   check  -- load if locking Emacs process is missing locally.
+
+If the value is nil, or `ask' and the user chooses not to load
+the desktop, the normal hook `desktop-not-loaded-hook' is run.
+
+If the value is `check', load the desktop if the Emacs process
+that has locked it is not running on the local machine.  This
+should not be used in circumstances where the locking Emacs might
+still be running on another machine.  That could be the case if
+you have remotely mounted (NFS) paths in `desktop-dirname'."
   :type
   '(choice
     (const :tag "Load anyway" t)
     (const :tag "Don't load" nil)
-    (const :tag "Ask the user" ask))
+    (const :tag "Ask the user" ask)
+    (const :tag "Load if no local process" check))
   :group 'desktop
   :version "22.2")
 
@@ -662,6 +671,27 @@ desktop-owner
 	     (integerp owner)))
 	 owner)))
 
+(defun desktop--emacs-pid-running-p (pid)
+  "Return t if an Emacs process with PID exists."
+  (when-let ((attr (process-attributes pid)))
+    (string-match "^emacs$" (alist-get 'comm attr))))
+
+(defun desktop--load-locked-desktop-p (owner)
+  "Return t if a locked desktop should be loaded.
+OWNER is the pid in the lock file.
+The return value of this function depends on the value of
+`desktop-load-locked-desktop'."
+  (pcase desktop-load-locked-desktop
+    ('ask
+     (unless (daemonp)
+       (y-or-n-p (format "Warning: desktop file appears to be in use by PID %s.\n\
+Using it may cause conflicts.  Use it anyway? " owner))))
+    ('check
+     (or (eq (emacs-pid) owner)
+         (not (desktop--emacs-pid-running-p owner))))
+    ('nil nil)
+    (_ t)))
+
 (defun desktop-claim-lock (&optional dirname)
   "Record this Emacs process as the owner of the desktop file in DIRNAME.
 DIRNAME omitted or nil means use `desktop-dirname'."
@@ -1238,11 +1268,7 @@ desktop-read
 	      (desktop-save nil)
 	      (desktop-autosave-was-enabled))
 	  (if (and owner
-		   (memq desktop-load-locked-desktop '(nil ask))
-		   (or (null desktop-load-locked-desktop)
-		       (daemonp)
-		       (not (y-or-n-p (format "Warning: desktop file appears to be in use by PID %s.\n\
-Using it may cause conflicts.  Use it anyway? " owner)))))
+                   (not (desktop--load-locked-desktop-p owner)))
 	      (let ((default-directory desktop-dirname))
 		(setq desktop-dirname nil)
 		(run-hooks 'desktop-not-loaded-hook)
diff --git a/test/lisp/desktop-tests.el b/test/lisp/desktop-tests.el
new file mode 100644
index 0000000000..7483bb8adb
--- /dev/null
+++ b/test/lisp/desktop-tests.el
@@ -0,0 +1,50 @@
+;;; desktop-tests.el --- Tests for desktop.el  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'desktop)
+
+(ert-deftest desktop-tests--emacs-pid-running-p ()
+  (should (desktop--emacs-pid-running-p (emacs-pid)))
+  (should-not (desktop--emacs-pid-running-p 1)))
+
+(ert-deftest desktop-tests--load-locked-desktop-p ()
+  (let ((desktop-load-locked-desktop t))
+    (should (desktop--load-locked-desktop-p (emacs-pid)))))
+
+(ert-deftest desktop-tests--load-locked-desktop-p-nil ()
+  (let ((desktop-load-locked-desktop nil))
+    (should-not (desktop--load-locked-desktop-p (emacs-pid)))))
+
+(ert-deftest desktop-tests--load-locked-desktop-p-ask ()
+ (let ((desktop-load-locked-desktop 'ask))
+   (cl-letf (((symbol-function 'y-or-n-p) (lambda (&rest _) t)))
+     (should (desktop--load-locked-desktop-p (emacs-pid))))
+   (cl-letf (((symbol-function 'y-or-n-p) (lambda (&rest _) nil)))
+     (should-not (desktop--load-locked-desktop-p (emacs-pid))))))
+
+(ert-deftest desktop-tests--load-locked-desktop-p-check ()
+  (let ((desktop-load-locked-desktop 'check))
+    (desktop--load-locked-desktop-p (emacs-pid))))
+
+(provide 'desktop-tests)
-- 
2.20.1

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> I agree with the proposal, and have written up a suggested patch.

No comments within 2 weeks.  If no one objects within a couple of
days, I intend to install this change on master (and consequently
close this bug).  Please voice any outstanding concerns before that.
Thanks.

Best regards,
Stefan Kangas

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 08 Feb 2020 15:23:21 +0100
> Cc: 1474@debbugs.gnu.org, emacs@gentoo.org
> 
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > I agree with the proposal, and have written up a suggested patch.
> 
> No comments within 2 weeks.  If no one objects within a couple of
> days, I intend to install this change on master (and consequently
> close this bug).  Please voice any outstanding concerns before that.

Sorry, I failed to present my comments at the time, so let me do that
now:

> +   t      -- load anyway.
> +   nil    -- don't load.
> +   ask    -- ask the user.
> +   check  -- load if locking Emacs process is missing locally.

I'd prefer 'check-pid' or maybe 'dead-pid' for this option.  "Check"
is too general.

> +If the value is `check', load the desktop if the Emacs process
> +that has locked it is not running on the local machine.  This
> +should not be used in circumstances where the locking Emacs might
> +still be running on another machine.  That could be the case if
> +you have remotely mounted (NFS) paths in `desktop-dirname'."

You are right in mentioning that this should not be used for processes
that run on other machines, but how can a user make sure this is not
the case?  Maybe we should modify the contents of the lock file to
include the host where the process was running, like we do with
file-locks?  Or maybe we should still ask for permission if there's no
such process, just with a different text, so that users who are sure
they _never_ run Emacs from another system could decide to unlock the
file more easily?

> +(defun desktop--emacs-pid-running-p (pid)
> +  "Return t if an Emacs process with PID exists."
> +  (when-let ((attr (process-attributes pid)))
> +    (string-match "^emacs$" (alist-get 'comm attr))))

I understand the rationale for the string-match test, but what if the
executable file name of Emacs was "transformed", per the
'--program-transform-name' option of the configure script?  And even
if it wasn't transformed, this will not match emacs-XX.YY and
emacs.exe.

Thanks.

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, I failed to present my comments at the time, so let me do that
> now:

Thank you kindly for the review.

> I'd prefer 'check-pid' or maybe 'dead-pid' for this option.  "Check"
> is too general.

Agreed.

> You are right in mentioning that this should not be used for processes
> that run on other machines, but how can a user make sure this is not
> the case?  Maybe we should modify the contents of the lock file to
> include the host where the process was running, like we do with
> file-locks?

OK, I'll get to work on this.

>              Or maybe we should still ask for permission if there's no
> such process, just with a different text, so that users who are sure
> they _never_ run Emacs from another system could decide to unlock the
> file more easily?

The motivation here was to have no prompt at all if we can avoid it.
So I think I'll get to work an the above suggestion instead, if that's
acceptable.

>> +(defun desktop--emacs-pid-running-p (pid)
>> +  "Return t if an Emacs process with PID exists."
>> +  (when-let ((attr (process-attributes pid)))
>> +    (string-match "^emacs$" (alist-get 'comm attr))))
>
> I understand the rationale for the string-match test, but what if the
> executable file name of Emacs was "transformed", per the
> '--program-transform-name' option of the configure script?  And even
> if it wasn't transformed, this will not match emacs-XX.YY and
> emacs.exe.

Good point.  But would you suggest to use no check at all, or is there
some other check we could reasonably use?  I assume it is impossible
to use the process name due to '--program-transform-name'.

Maybe we could think about prompting for the case where the pid
exists, but the name doesn't match "^emacs"?  The user should know
best if this is a renamed process or what.

Best regards,
Stefan Kangas

bug#1474: 23.0.60; desktop.el don't check if pid in his lock file is always in use

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Cc: emacs@gentoo.org,  1474@debbugs.gnu.org,  thierry.volpiatto@gmail.com
> Date: Mon, 27 Apr 2020 12:34:08 +0200
> 
> >> +(defun desktop--emacs-pid-running-p (pid)
> >> +  "Return t if an Emacs process with PID exists."
> >> +  (when-let ((attr (process-attributes pid)))
> >> +    (string-match "^emacs$" (alist-get 'comm attr))))
> >
> > I understand the rationale for the string-match test, but what if the
> > executable file name of Emacs was "transformed", per the
> > '--program-transform-name' option of the configure script?  And even
> > if it wasn't transformed, this will not match emacs-XX.YY and
> > emacs.exe.
> 
> Good point.  But would you suggest to use no check at all, or is there
> some other check we could reasonably use?  I assume it is impossible
> to use the process name due to '--program-transform-name'.

Perhaps you could use the name you find in command-line-args?  Note
that it might include leading directories, and at least on Windows the
directory separator might either be a slash or a backslash, so use of
file-name-nondirectory is advised.

bug#1474: bug#25232: 24.4; Eliminate - Warning: Desktop file appears to be in use

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Perhaps you could use the name you find in command-line-args?  Note
> that it might include leading directories, and at least on Windows the
> directory separator might either be a slash or a backslash, so use of
> file-name-nondirectory is advised.

I've now applied Stefan's patch to Emacs 29 (adjusting for Eli's
comments).  There's still the question of altering the desktop format to
identify the machine it's running on, but I think Stefan's patch covers
most of the issue discussed, if I understand correctly.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no