Re: [PATCH] xf86-input-synaptics: fix creation of repeater fifo
On Monday 29 September 2008, Peter Hutterer wrote: On Sun, Sep 28, 2008 at 03:11:46PM +0100, Magnus Kessler wrote: Testing status against EEXIST is wrong and we should check errno instead if we want to allow the use of an existing file. However, since we pass a file name that in principle could be any existing file (not just fifos) is there any guarantee that we can later actually use the fifo? Thanks. There is no guarantee that we can use it, but at the same time the use-case where the pipe already exists is common. In the simple case of a server restart, the first mkfifo command succeeds but the second fails with EEXIST. So the pipe is still there and should be used. Admittedly, it might be a good idea to clean up after ourselves and delete the fifo if we have created it in the first place. What about the (compile-tested) code below? Cheers, Peter [patch snipped] Hi Peter, your patch would certainly work and prevent fifos created by the driver hang around if and when the X server terminated normally. Crashes would still lead to the current situation. I'm also slightly nervous about adding yet more code to functionality that is effectively never used in normal operations. New proposal to follow... Cheers, Magnus signature.asc Description: This is a digitally signed message part. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH] xf86-input-synaptics: fix creation of repeater fifo
On Sun, Sep 28, 2008 at 10:09:01AM +0100, Magnus Kessler wrote:
The creation of the repeater fifo in the synaptics driver looks dubious. The
file mode should be ORed with the S_IFIFO flag and the dev parameter should
be null. The mknod(3p) man page suggests using mkfifo instead.
Agreed.
[...]
repeater = xf86SetStrOption(local-options, Repeater, NULL);
if (repeater) {
/* create repeater fifo */
- status = mknod(repeater, 666, S_IFIFO);
- if ((status != 0) (status != EEXIST)) {
+ if (mkfifo(repeater, S_IWUSR|S_IRUSR|S_IWGRP|S_IRGRP|S_IWOTH|S_IROTH)) {
xf86Msg(X_ERROR, %s can't create repeater fifo\n, local-name);
} else {
/* open the repeater fifo */
Any special reason for dropping the EEXIST case?
Cheers,
Peter
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH] xf86-input-synaptics: fix creation of repeater fifo
On Sun, Sep 28, 2008 at 03:11:46PM +0100, Magnus Kessler wrote:
Testing status against EEXIST is wrong and we should check errno instead if
we want to allow the use of an existing file. However, since we pass a file
name that in principle could be any existing file (not just fifos) is there
any guarantee that we can later actually use the fifo?
Thanks. There is no guarantee that we can use it, but at the same time the
use-case where the pipe already exists is common.
In the simple case of a server restart, the first mkfifo command succeeds but
the second fails with EEXIST. So the pipe is still there and should be used.
Admittedly, it might be a good idea to clean up after ourselves and delete the
fifo if we have created it in the first place. What about the (compile-tested)
code below?
Cheers,
Peter
diff --git a/src/synaptics.c b/src/synaptics.c
index 18168e0..e5cb7f0 100644
--- a/src/synaptics.c
+++ b/src/synaptics.c
@@ -512,6 +512,9 @@ static void set_repeater_fifo(LocalDevicePtr local)
if (status (errno != EEXIST)) {
xf86Msg(X_ERROR, %s can't create repeater fifo\n, local-name);
} else {
+if (!status)
+priv-fifo_path = strdup(repeater); /* for unlinking later */
+
/* open the repeater fifo */
optList = xf86NewOption(Device, repeater);
if ((priv-fifofd = xf86OpenSerial(optList)) == -1) {
@@ -608,6 +611,7 @@ SynapticsPreInit(InputDriverPtr drv, IDevPtr dev, int flags)
DBG(9, XisbTrace(priv-comm.buffer, 1));
priv-fifofd = -1;
+priv-fifo_path = NULL;
set_repeater_fifo(local);
if (!QueryHardware(local)) {
@@ -655,6 +659,13 @@ static void SynapticsUnInit(InputDriverPtr drv,
InputInfoPtr local,
intflags)
{
+SynapticsPrivate *priv = (SynapticsPrivate *) (local-private);
+
+if (priv-fifo_path)
+{
+unlink(priv-fifo_path);
+xfree(priv-fifo_path);
+}
xfree(local-private);
local-private = NULL;
xf86DeleteInput(local, 0);
diff --git a/src/synapticsstr.h b/src/synapticsstr.h
index e5202d1..d93e279 100644
--- a/src/synapticsstr.h
+++ b/src/synapticsstr.h
@@ -96,6 +96,7 @@ typedef struct _SynapticsPrivateRec
struct CommData comm;
int fifofd;/* fd for fifo */
+char *fifo_path;/* path to fifo for unlinking */
SynapticsMoveHistRec move_hist[SYNAPTICS_MOVE_HISTORY]; /* movement
history */
int hist_index;/* Last added entry in move_hist[] */
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
