From: Noam Postavsky <npostavs@gmail.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 33016@debbugs.gnu.org, Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
Subject: bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist
Date: Fri, 12 Apr 2019 14:20:46 -0400 [thread overview]
Message-ID: <CAM-tV-_YETRKx=5LPcMOVqFTPWXD0cY=HOBzAh1L0v_8x_6+eA@mail.gmail.com> (raw)
In-Reply-To: <83imvjmx32.fsf@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 875 bytes --]
On Fri, 12 Apr 2019 at 04:45, Eli Zaretskii <eliz@gnu.org> wrote:
> > + ;; On Windows, "nul.FOO" is the empty file for any
> > + ;; FOO, in any directory. So this passes Emacs'
> > + ;; test for the file's existence, and ensures we
> > + ;; hit an error in the w32 process spawn code.
> > + (call-process "c:/nul.exe")
>
> What happened to mentioning the null device in this comment?
Yeah, I forgot to change this comment (though I don't think
specifically naming the null device is needed).
> > + (setq when-entered-debugger -1))))
>
> This should be internal-when-entered-debugger, right? And the same in
> the other test.
Oops, right, that's what I get for rushing things. I think got
everything this time (include replacing RMS' question in eval.c).
[-- Attachment #2: v5-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/octet-stream, Size: 9322 bytes --]
From b85d57e7d5e0201502d9fa4f933334da75ef2a61 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 17:57:22 -0400
Subject: [PATCH v5] Let debugger handle process spawn errors on w32
(Bug#33016)
Since child_setup() is called between block_input()...unblock_input(),
when an error is signaled the Lisp debugger is prevented from
starting. Therefore, let the callers signal the error instead (which
they already do for non-w32 platforms, just the error message needs an
update).
* src/callproc.c (child_setup) [WINDOWSNT]: Don't call
report_file_error here.
(call_process) [WINDOWNT]:
* src/process.c (create_process) [WINDOWSNT]: Call report_file_errno
here instead, after the unblock_input() call, same as for !WINDOWSNT.
* src/lisp.h (CHILD_SETUP_ERROR_DESC): New preprocessor define. Flip
the containing ifndef DOS_NT branches so that it's ifdef DOS_NT.
* src/eval.c (when_entered_debugger): Remove.
(syms_of_eval) <internal-when-entered-debugger>: Define it as a Lisp
integer variable instead.
(maybe_call_debugger): Update comment.
* test/src/process-tests.el (make-process-w32-debug-spawn-error):
* test/src/callproc-tests.el (call-process-w32-debug-spawn-error): New
tests.
---
src/callproc.c | 7 ++-----
src/eval.c | 24 ++++++++++++++----------
src/lisp.h | 9 ++++++---
src/process.c | 2 +-
test/src/callproc-tests.el | 22 ++++++++++++++++++++++
test/src/process-tests.el | 20 ++++++++++++++++++++
6 files changed, 65 insertions(+), 19 deletions(-)
diff --git a/src/callproc.c b/src/callproc.c
index a3d0960..2cdf84d 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -681,7 +681,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
unblock_input ();
if (pid < 0)
- report_file_errno ("Doing vfork", Qnil, child_errno);
+ report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, child_errno);
/* Close our file descriptors, except for callproc_fd[CALLPROC_PIPEREAD]
since we will use that to read input from. */
@@ -1174,7 +1174,7 @@ exec_failed (char const *name, int err)
executable directory by the parent.
On GNUish hosts, either exec or return an error number.
- On MS-Windows, either return a pid or signal an error.
+ On MS-Windows, either return a pid or return -1 and set errno.
On MS-DOS, either return an exit status or signal an error. */
CHILD_SETUP_TYPE
@@ -1319,9 +1319,6 @@ child_setup (int in, int out, int err, char **new_argv, bool set_pgrp,
/* Spawn the child. (See w32proc.c:sys_spawnve). */
cpid = spawnve (_P_NOWAIT, new_argv[0], new_argv, env);
reset_standard_handles (in, out, err, handles);
- if (cpid == -1)
- /* An error occurred while trying to spawn the process. */
- report_file_error ("Spawning child process", Qnil);
return cpid;
#else /* not WINDOWSNT */
diff --git a/src/eval.c b/src/eval.c
index e9f118c..fa7b2d0 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -52,15 +52,6 @@ Lisp_Object Vautoload_queue;
is shutting down. */
Lisp_Object Vrun_hooks;
-/* The value of num_nonmacro_input_events as of the last time we
- started to enter the debugger. If we decide to enter the debugger
- again when this is still equal to num_nonmacro_input_events, then we
- know that the debugger itself has an error, and we should just
- signal the error instead of entering an infinite loop of debugger
- invocations. */
-
-static intmax_t when_entered_debugger;
-
/* The function from which the last `signal' was called. Set in
Fsignal. */
/* FIXME: We should probably get rid of this! */
@@ -1835,7 +1826,8 @@ maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig, Lisp_Object data)
? debug_on_quit
: wants_debugger (Vdebug_on_error, conditions))
&& ! skip_debugger (conditions, combined_data)
- /* RMS: What's this for? */
+ /* See commentary on definition of
+ `internal-when-entered-debugger'. */
&& when_entered_debugger < num_nonmacro_input_events)
{
call_debugger (list2 (Qerror, combined_data));
@@ -4170,6 +4162,18 @@ Note that `debug-on-error', `debug-on-quit' and friends
still determine whether to handle the particular condition. */);
Vdebug_on_signal = Qnil;
+ /* The value of num_nonmacro_input_events as of the last time we
+ started to enter the debugger. If we decide to enter the debugger
+ again when this is still equal to num_nonmacro_input_events, then we
+ know that the debugger itself has an error, and we should just
+ signal the error instead of entering an infinite loop of debugger
+ invocations. */
+ DEFSYM (Qinternal_when_entered_debugger, "internal-when-entered-debugger");
+ DEFVAR_INT ("internal-when-entered-debugger", when_entered_debugger,
+ doc: /* The number of keyboard events as of last time `debugger' was called.
+Used to avoid infinite loops if the debugger itself has an error.
+Don't set this unless you're sure that can't happen. */);
+
/* When lexical binding is being used,
Vinternal_interpreter_environment is non-nil, and contains an alist
of lexically-bound variable, or (t), indicating an empty
diff --git a/src/lisp.h b/src/lisp.h
index 681efc3..2915944 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4480,11 +4480,14 @@ extern void syms_of_process (void);
extern void setup_process_coding_systems (Lisp_Object);
/* Defined in callproc.c. */
-#ifndef DOS_NT
-# define CHILD_SETUP_TYPE _Noreturn void
-#else
+#ifdef DOS_NT
# define CHILD_SETUP_TYPE int
+# define CHILD_SETUP_ERROR_DESC "Spawning child process"
+#else
+# define CHILD_SETUP_TYPE _Noreturn void
+# define CHILD_SETUP_ERROR_DESC "Doing vfork"
#endif
+
extern CHILD_SETUP_TYPE child_setup (int, int, int, char **, bool, Lisp_Object);
extern void init_callproc_1 (void);
extern void init_callproc (void);
diff --git a/src/process.c b/src/process.c
index 6770a5e..0c44037 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2233,7 +2233,7 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
unblock_input ();
if (pid < 0)
- report_file_errno ("Doing vfork", Qnil, vfork_errno);
+ report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, vfork_errno);
else
{
/* vfork succeeded. */
diff --git a/test/src/callproc-tests.el b/test/src/callproc-tests.el
index 7b30a25..f351b6e 100644
--- a/test/src/callproc-tests.el
+++ b/test/src/callproc-tests.el
@@ -37,3 +37,25 @@
(split-string-and-unquote (buffer-string)))
(should (equal initial-shell "nil"))
(should-not (equal initial-shell shell))))
+
+(ert-deftest call-process-w32-debug-spawn-error ()
+ "Check that debugger runs on `call-process' failure (Bug#33016)."
+ (skip-unless (eq system-type 'windows-nt))
+ (let* ((debug-on-error t)
+ (have-called-debugger nil)
+ (debugger (lambda (&rest _)
+ (setq have-called-debugger t)
+ ;; Allow entering the debugger later in the same
+ ;; test run, before going back to the command
+ ;; loop.
+ (setq internal-when-entered-debugger -1))))
+ (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+ (condition-case-unless-debug ()
+ ;; On Windows, "nul.FOO" act like an always-empty
+ ;; file for any FOO, in any directory. So this
+ ;; passes Emacs' test for the file's existence,
+ ;; and ensures we hit an error in the w32 process
+ ;; spawn code.
+ (call-process "c:/nul.exe")
+ (error :got-error))))
+ (should have-called-debugger)))
diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 5dbf441..0bb7ebe 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -215,6 +215,26 @@ process-tests--mixable
(string-to-list "stdout\n")
(string-to-list "stderr\n"))))))
+(ert-deftest make-process-w32-debug-spawn-error ()
+ "Check that debugger runs on `make-process' failure (Bug#33016)."
+ (skip-unless (eq system-type 'windows-nt))
+ (let* ((debug-on-error t)
+ (have-called-debugger nil)
+ (debugger (lambda (&rest _)
+ (setq have-called-debugger t)
+ ;; Allow entering the debugger later in the same
+ ;; test run, before going back to the command
+ ;; loop.
+ (setq internal-when-entered-debugger -1))))
+ (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+ (condition-case-unless-debug ()
+ ;; Emacs doesn't search for absolute filenames, so
+ ;; the error will be hit in the w32 process spawn
+ ;; code.
+ (make-process :name "test" :command '("c:/No-Such-Command"))
+ (error :got-error))))
+ (should have-called-debugger)))
+
(ert-deftest make-process/file-handler/found ()
"Check that the ‘:file-handler’ argument of ‘make-process’
works as expected if a file name handler is found."
--
2.6.2.windows.1
next prev parent reply other threads:[~2019-04-12 18:20 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-11 12:55 bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist Klaus-Dieter Bauer
2018-10-11 14:22 ` Eli Zaretskii
2018-10-19 8:03 ` Klaus-Dieter Bauer
2018-10-19 8:30 ` Eli Zaretskii
2019-04-08 18:34 ` Noam Postavsky
2019-04-08 18:58 ` Eli Zaretskii
2019-04-09 14:13 ` Noam Postavsky
2019-04-09 14:33 ` Eli Zaretskii
2019-04-10 21:58 ` Noam Postavsky
2019-04-11 14:04 ` Eli Zaretskii
2019-04-11 17:34 ` Noam Postavsky
2019-04-11 17:55 ` Eli Zaretskii
2019-04-12 0:44 ` Noam Postavsky
2019-04-12 8:44 ` Eli Zaretskii
2019-04-12 18:20 ` Noam Postavsky [this message]
2019-04-12 18:44 ` Eli Zaretskii
2019-04-15 12:21 ` Noam Postavsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM-tV-_YETRKx=5LPcMOVqFTPWXD0cY=HOBzAh1L0v_8x_6+eA@mail.gmail.com' \
--to=npostavs@gmail.com \
--cc=33016@debbugs.gnu.org \
--cc=bauer.klaus.dieter@gmail.com \
--cc=eliz@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.