Re: xidle: do not close stdout/err, error on failure, execvp(3)
On Sat, Nov 17 2018, Klemens Nanni wrote:
> On Sun, Nov 11, 2018 at 05:39:52PM +0100, Klemens Nanni wrote:
>> On Sat, Nov 03, 2018 at 09:01:33PM +0100, Klemens Nanni wrote:
>> > Closing stdin makes sense, but I still want to see error messages from
>> > the program I'm running. Since arbitrary progams can be run, keep both
>> > stdout and stderr open so users get a chance to actually notice program
>> > failure or maybe even use output for good.
>> >
>> > In a common setup where xidle(1) is started from ~/.xsession, I'd expect
>> > errors to pop up in ~/.xsession-errors.
I think the daemon(3)-like behavior was intended. On the 3628 lines in
my ~/.xsession-errors, 3566 of them are useless crap from a single
program.
>> This, plus closely related changes:
>>
>> We should never execute the program unless a new session was
>> created so that the child process does not share the same controlling
>> terminal.
xidle might not have a controlling terminal in the first place, for
example when spawned by my window manager. You're adding a possible
failure case which might lead to undesirable consequences, eg failure to
lock the X session. This has to be weighed in.
>> Also, use execvp(3) to search PATH so users don't have to provide full
>> paths any longer. Not sure why this wasn't done in the first place.
>
>> Termination information from wait(2) is not used and irrelevant at this
>> point, so zap `status'.
> New diff with complete "full path" -> "program" changes in the manual
> this time. Shuffling/splitting diffs got a bit messy here, sorry.
>
> I'm running with this on my daily work machines without any problems.
>
> Feedback? OK?
>
> Index: xidle.1
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.1,v
> retrieving revision 1.5
> diff -u -p -r1.5 xidle.1
> --- xidle.1 6 Sep 2018 07:21:34 - 1.5
> +++ xidle.1 17 Nov 2018 12:11:29 -
> @@ -23,7 +23,7 @@
> .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> .\" OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> .\"
> -.Dd June 20, 2005
> +.Dd $Mdocdate: November 17 2018 $
> .Dt XIDLE 1
> .Os
> .Sh NAME
> @@ -36,7 +36,7 @@
> .Op Fl delay Ar secs
> .Op Fl display Ar display
> .Op Fl no | nw | ne | sw | se
> -.Op Fl program Ar path
> +.Op Fl program Ar program
> .Op Fl timeout Ar secs
> .Ek
> .Sh DESCRIPTION
> @@ -71,9 +71,8 @@ Set the position to one of none, northwe
> respectively.
> If no position is specified,
> the default is northwest.
> -.It Fl program Ar path
> -Specify the full pathname of the program to run on any of the
> -aforementioned events.
> +.It Fl program Ar program
> +Specify the program to run on any of the aforementioned events.
> Arguments to the program may also be specified, separated by whitespace.
> If
> .Fl program
> @@ -110,7 +109,7 @@ and
> .Fl se
> options.
> .It Sy program No (class Sy Program )
> -Specify the full pathname of the program to run; see the
> +Specify the program to run; see the
> .Fl program
> option.
> .It Sy timeout No (class Sy Timeout )
> @@ -129,8 +128,7 @@ Run
> using the flying bats mode if no activity is detected in 300 seconds or the
> pointer sits in the southwest corner for more than 5 seconds:
> .Bd -literal -offset indent
> -$ xidle -delay 5 -sw -program "/usr/X11R6/bin/xlock -mode bat" \e
> - -timeout 300
> +$ xidle -delay 5 -sw -program "xlock -mode bat" -timeout 300
> .Ed
> .Sh SEE ALSO
> .Xr xlock 1 ,
> Index: xidle.c
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 xidle.c
> --- xidle.c 11 Nov 2018 16:10:37 - 1.8
> +++ xidle.c 17 Nov 2018 12:17:55 -
> @@ -94,7 +94,7 @@ static XrmOptionDescRec opts[] = {
>
> extern char *__progname;
>
> -void action(struct xinfo *, char **);
> +void action(struct xinfo *, char *const []);
> void close_x(struct xinfo *);
> Bool getres(XrmValue *, const XrmDatabase, const char *, const char *);
> voidinit_x(struct xinfo *, int, int, int);
> @@ -180,19 +180,18 @@ close_x(struct xinfo *xi)
>
>
> void
> -action(struct xinfo *xi, char **args)
> +action(struct xinfo *xi, char *const args[])
> {
> - int dumb;
> -
> switch (fork()) {
> case -1:
> err(1, "fork");
> case 0:
> - setsid();
> - execv(*args, args);
> - exit(1);
> + if (setsid() == -1)
> + err(1, "setsid");
> + execvp(args[0], args);
> + err(1, "execvp: %s", args[0]);
> default:
> - wait();
> + wait(NULL);
> XSync(xi->dpy, True);
> break;
> }
> @@ -356,8 +355,6 @@ main(int argc, char **argv)
> if (fd < 0)
> err(1, _PATH_DEVNULL);
> dup2(fd, STDIN_FILENO);
> - dup2(fd, STDOUT_FILENO);
> -
Re: xidle: do not close stdout/err, error on failure, execvp(3)
On Sun, Nov 11, 2018 at 05:39:52PM +0100, Klemens Nanni wrote:
> On Sat, Nov 03, 2018 at 09:01:33PM +0100, Klemens Nanni wrote:
> > Closing stdin makes sense, but I still want to see error messages from
> > the program I'm running. Since arbitrary progams can be run, keep both
> > stdout and stderr open so users get a chance to actually notice program
> > failure or maybe even use output for good.
> >
> > In a common setup where xidle(1) is started from ~/.xsession, I'd expect
> > errors to pop up in ~/.xsession-errors.
> This, plus closely related changes:
>
> We should never execute the program unless a new session was
> created so that the child process does not share the same controlling
> terminal.
>
> Also, use execvp(3) to search PATH so users don't have to provide full
> paths any longer. Not sure why this wasn't done in the first place.
>
> Termination information from wait(2) is not used and irrelevant at this
> point, so zap `status'.
New diff with complete "full path" -> "program" changes in the manual
this time. Shuffling/splitting diffs got a bit messy here, sorry.
I'm running with this on my daily work machines without any problems.
Feedback? OK?
Index: xidle.1
===
RCS file: /cvs/xenocara/app/xidle/xidle.1,v
retrieving revision 1.5
diff -u -p -r1.5 xidle.1
--- xidle.1 6 Sep 2018 07:21:34 - 1.5
+++ xidle.1 17 Nov 2018 12:11:29 -
@@ -23,7 +23,7 @@
.\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
.\" OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
.\"
-.Dd June 20, 2005
+.Dd $Mdocdate: November 17 2018 $
.Dt XIDLE 1
.Os
.Sh NAME
@@ -36,7 +36,7 @@
.Op Fl delay Ar secs
.Op Fl display Ar display
.Op Fl no | nw | ne | sw | se
-.Op Fl program Ar path
+.Op Fl program Ar program
.Op Fl timeout Ar secs
.Ek
.Sh DESCRIPTION
@@ -71,9 +71,8 @@ Set the position to one of none, northwe
respectively.
If no position is specified,
the default is northwest.
-.It Fl program Ar path
-Specify the full pathname of the program to run on any of the
-aforementioned events.
+.It Fl program Ar program
+Specify the program to run on any of the aforementioned events.
Arguments to the program may also be specified, separated by whitespace.
If
.Fl program
@@ -110,7 +109,7 @@ and
.Fl se
options.
.It Sy program No (class Sy Program )
-Specify the full pathname of the program to run; see the
+Specify the program to run; see the
.Fl program
option.
.It Sy timeout No (class Sy Timeout )
@@ -129,8 +128,7 @@ Run
using the flying bats mode if no activity is detected in 300 seconds or the
pointer sits in the southwest corner for more than 5 seconds:
.Bd -literal -offset indent
-$ xidle -delay 5 -sw -program "/usr/X11R6/bin/xlock -mode bat" \e
- -timeout 300
+$ xidle -delay 5 -sw -program "xlock -mode bat" -timeout 300
.Ed
.Sh SEE ALSO
.Xr xlock 1 ,
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.8
diff -u -p -r1.8 xidle.c
--- xidle.c 11 Nov 2018 16:10:37 - 1.8
+++ xidle.c 17 Nov 2018 12:17:55 -
@@ -94,7 +94,7 @@ static XrmOptionDescRec opts[] = {
extern char *__progname;
-void action(struct xinfo *, char **);
+void action(struct xinfo *, char *const []);
void close_x(struct xinfo *);
Bool getres(XrmValue *, const XrmDatabase, const char *, const char *);
voidinit_x(struct xinfo *, int, int, int);
@@ -180,19 +180,18 @@ close_x(struct xinfo *xi)
void
-action(struct xinfo *xi, char **args)
+action(struct xinfo *xi, char *const args[])
{
- int dumb;
-
switch (fork()) {
case -1:
err(1, "fork");
case 0:
- setsid();
- execv(*args, args);
- exit(1);
+ if (setsid() == -1)
+ err(1, "setsid");
+ execvp(args[0], args);
+ err(1, "execvp: %s", args[0]);
default:
- wait();
+ wait(NULL);
XSync(xi->dpy, True);
break;
}
@@ -356,8 +355,6 @@ main(int argc, char **argv)
if (fd < 0)
err(1, _PATH_DEVNULL);
dup2(fd, STDIN_FILENO);
- dup2(fd, STDOUT_FILENO);
- dup2(fd, STDERR_FILENO);
if (fd > 2)
close(fd);
Re: xidle: do not close stdout/err, error on failure, execvp(3)
On Sat, Nov 03, 2018 at 09:01:33PM +0100, Klemens Nanni wrote:
> Closing stdin makes sense, but I still want to see error messages from
> the program I'm running. Since arbitrary progams can be run, keep both
> stdout and stderr open so users get a chance to actually notice program
> failure or maybe even use output for good.
>
> In a common setup where xidle(1) is started from ~/.xsession, I'd expect
> errors to pop up in ~/.xsession-errors.
This, plus closely related changes:
We should never execute the program unless a new session was
created so that the child process does not share the same controlling
terminal.
Also, use execvp(3) to search PATH so users don't have to provide full
paths any longer. Not sure why this wasn't done in the first place.
Termination information from wait(2) is not used and irrelevant at this
point, so zap `status'.
OK?
Index: xidle.1
===
RCS file: /cvs/xenocara/app/xidle/xidle.1,v
retrieving revision 1.5
diff -u -p -r1.5 xidle.1
--- xidle.1 6 Sep 2018 07:21:34 - 1.5
+++ xidle.1 11 Nov 2018 16:36:10 -
@@ -72,8 +72,7 @@ respectively.
If no position is specified,
the default is northwest.
.It Fl program Ar path
-Specify the full pathname of the program to run on any of the
-aforementioned events.
+Specify the program to run on any of the aforementioned events.
Arguments to the program may also be specified, separated by whitespace.
If
.Fl program
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.8
diff -u -p -r1.8 xidle.c
--- xidle.c 11 Nov 2018 16:10:37 - 1.8
+++ xidle.c 11 Nov 2018 16:36:10 -
@@ -94,7 +94,7 @@ static XrmOptionDescRec opts[] = {
extern char *__progname;
-void action(struct xinfo *, char **);
+void action(struct xinfo *, char *const []);
void close_x(struct xinfo *);
Bool getres(XrmValue *, const XrmDatabase, const char *, const char *);
voidinit_x(struct xinfo *, int, int, int);
@@ -180,19 +180,18 @@ close_x(struct xinfo *xi)
void
-action(struct xinfo *xi, char **args)
+action(struct xinfo *xi, char *const args[])
{
- int dumb;
-
switch (fork()) {
case -1:
err(1, "fork");
case 0:
- setsid();
- execv(*args, args);
- exit(1);
+ if (setsid() == -1)
+ err(1, "setsid");
+ execvp(args[0], args);
+ err(1, "execvp");
default:
- wait();
+ wait(NULL);
XSync(xi->dpy, True);
break;
}
@@ -356,8 +355,6 @@ main(int argc, char **argv)
if (fd < 0)
err(1, _PATH_DEVNULL);
dup2(fd, STDIN_FILENO);
- dup2(fd, STDOUT_FILENO);
- dup2(fd, STDERR_FILENO);
if (fd > 2)
close(fd);
