scrot exits success on error

[guijan]

Scrot exits success even if the -e option fails: https://github.com/resurrecting-open-source-projects/scrot/blob/75664efd19d02c0d70006e902364a877bec902b9/src/scrot.c#L572-L579

[guijan]

I also just noticed that you can make your command this:

scrot -e 'return 127'

Scrot is going to incorrectly complain it couldn't execute the command. The POSIX shell may be defined to return 127 if it can't find the command, but nothing stops the last program in shell script from returning 127 and thus making the shell return 127.

Also, the shell will probably print an error message for us anyway, so that is redundant:

[N-R-K]

What's the sensible thing to do if the child exits with non-zero status? Should we mirror it? But then again non-zero exit status doesn't necessarily mean an error and some programs use exit status to signal different things (e.g test(1)) which aren't exactly errors.

[guijan]

We could mirror it & not print anything. That would fix all the issues mentioned here. But then it raises the issue of not knowing whether a non-0 return is from scrot or the command.

Or we could return 0 if scrot & the command succeed, 1 if scrot errors, 2 if the command returns non-0. Then, the exact return value of the command isn't returned, but you can tell who returned non-0. It's okay if scrot -e has limitations, if someone wants something more powerful he can write a shell script.

[N-R-K]

Rough patch:

diff --git a/src/scrot.c b/src/scrot.c
index 3cb1dcf..9bf56be 100644
--- a/src/scrot.c
+++ b/src/scrot.c
@@ -644,11 +644,10 @@ static void scrotExecApp(Imlib_Image image, struct tm *tm, char *filenameIM,
 {
     char *execStr = imPrintf(opt.exec, tm, filenameIM, filenameThumb, image);
     int ret = system(execStr);
-
     if (ret == -1)
-        warn("The child process could not be created");
-    else if (WIFEXITED(ret) && WEXITSTATUS(ret) == 127)
-        warnx("scrot could not execute the command: %s", execStr);
+        err(EXIT_FAILURE, "The child process could not be created");
+    else if (WIFEXITED(ret) && WEXITSTATUS(ret) != 0)
+        exit(69 /* EX_UNAVAILABLE */ );
     free(execStr);
 }

The EX_UNAVAILABLE constant is from freebsd's sysexit convention, which describes it as:

This can also be used as a catchall message when something you wanted to do does not work, but you do not know why.

[guijan]

Looks good. Also, the code doesn't currently handle the child being terminated by a signal:

if (WIFSIGNALED(ret))
    errx(1, "Child killed by signal: %s", strsignal(WTERMSIG(ret)));

[N-R-K]

Any reason for special casing signal termination? If not, then we could just do !WIFEXITED(ret) || WEXITSTATUS(ret) != 0 instead.

EDIT: See https://github.com/resurrecting-open-source-projects/scrot/pull/370

[guijan]

https://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/wait.h.html

POSIX only specifies what WEXITSTATUS() does if WIFEXITED() returns true:

 WEXITSTATUS(stat_val)
             If the value of WIFEXITED(stat_val) is non-zero, this
             macro evaluates to the low-order 8 bits of the status
             argument

WIFEXITED() and WIFSIGNALED() are mutually exclusive if neither WUNTRACED nor WCONTINUED are passed to waitpid():

If the information pointed to by stat_loc was stored by a call 
to waitpid() that did not specify the WUNTRACED or WCONTINUED 
flags, or by a call to the wait() function, exactly one of the
macros WIFEXITED(*stat_loc) and WIFSIGNALED(*stat_loc) shall 
evaluate to a non-zero value.

Now, if we look at the definition of WUNTRACED and WCONTINUED:

 WCONTINUED    Report status of continued
               child process.
 WUNTRACED     Report status of stopped child
               process.

We can see that they can't be used from the implementation of system() because they would make system() return for a child that hasn't terminated: https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functions/system.html

The system() function shall not return until
the child process has terminated.