all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
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: Tue, 9 Apr 2019 10:13:58 -0400	[thread overview]
Message-ID: <CAM-tV-8VKzCT0n3=rwmViLR8uP=snOYranhuhuFsiif71Ox8fQ@mail.gmail.com> (raw)
In-Reply-To: <83y34k728d.fsf@gnu.org>

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

On Mon, 8 Apr 2019 at 14:59, Eli Zaretskii <eliz@gnu.org> wrote:

> > The confusing thing here is that the error is signaled between
> > block_input()...unblock_input(), which prevents the debugger from
> > triggering.

> > The attached patch fixes this by moving the signal to after the unblock_input().
>
> Thanks, but could we have a test for this, please?

Yes (I had initially thought it wouldn't work because of the way ert
uses the debugger internally, but it actually turns out fine).

By the way, I modified the error message in call_process in addition
to create_process for completeness, but I can't see a way to trigger
this for call_process: it searches for PROGRAM and signals an error
early, regardless of whether the filename is absolute or not.

[-- Attachment #2: v2-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/octet-stream, Size: 4513 bytes --]

From d7762f2599cbd2fed1b661b60dacf6fd444a7f39 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 17:57:22 -0400
Subject: [PATCH v2] 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.
* test/src/process-tests.el (make-process-w32-spawn-nonexistent): New
test.
---
 src/callproc.c            |  7 ++-----
 src/lisp.h                |  6 ++++++
 src/process.c             |  2 +-
 test/src/process-tests.el | 12 ++++++++++++
 4 files changed, 21 insertions(+), 6 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/lisp.h b/src/lisp.h
index a0a7cbd..6f39265 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4477,6 +4477,12 @@ extern void setup_process_coding_systems (Lisp_Object);
 #else
 # define CHILD_SETUP_TYPE int
 #endif
+#ifdef WINDOWSNT
+# define CHILD_SETUP_ERROR_DESC "Spawning child process"
+#else
+# 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 802ac02..ff08657 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2232,7 +2232,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/process-tests.el b/test/src/process-tests.el
index 5dbf441..779665ad 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -215,6 +215,18 @@ process-tests--mixable
                                       (string-to-list "stdout\n")
                                       (string-to-list "stderr\n"))))))
 
+(ert-deftest make-process-w32-spawn-nonexistent ()
+  "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))))
+    (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+                (condition-case-unless-debug ()
+                    (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


  reply	other threads:[~2019-04-09 14:13 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 [this message]
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
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-8VKzCT0n3=rwmViLR8uP=snOYranhuhuFsiif71Ox8fQ@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.