From: Chris W. <cw...@f0...> - 2004-11-15 03:25:54
|
If UML wants to open an xterm channel and the xterm does not run properly (eg. terminates soon after starting) we will get a hang. This avoids the most common cause for this and adds a comment (which long term will go away with a rewrite of that code I guess?) Signed-off-by: Chris Wedgwood <cw...@f0...> --- arch/um/drivers/xterm.c | 7 +++++++ arch/um/drivers/xterm_kern.c | 3 +++ 2 files changed, 10 insertions(+) Index: cw-current/arch/um/drivers/xterm.c =================================================================== --- cw-current.orig/arch/um/drivers/xterm.c 2004-11-14 17:52:05.955194653 -0800 +++ cw-current/arch/um/drivers/xterm.c 2004-11-14 17:52:23.048945524 -0800 @@ -94,6 +94,13 @@ "/usr/lib/uml/port-helper", "-uml-socket", file, NULL }; + /* Check that DISPLAY is set, this doesn't guarantee the xterm + * will work but w/o it we can be pretty sure it won't. */ + if (!getenv("DISPLAY")) { + printk("xterm_open: $DISPLAY not set.\n"); + return -ENODEV; + } + if(os_access(argv[4], OS_ACC_X_OK) < 0) argv[4] = "port-helper"; Index: cw-current/arch/um/drivers/xterm_kern.c =================================================================== --- cw-current.orig/arch/um/drivers/xterm_kern.c 2004-11-14 17:52:51.484194558 -0800 +++ cw-current/arch/um/drivers/xterm_kern.c 2004-11-14 17:53:09.197972625 -0800 @@ -61,6 +61,9 @@ ret = err; goto out; } + + /* XXX Note, if the xterm doesn't work for some reason + * (eg. DISPLAY isn't set this will hang... */ down(&data->sem); free_irq_by_irq_and_dev(XTERM_IRQ, data); |
From: Blaisorblade <bla...@ya...> - 2004-11-15 17:22:57
|
On Monday 15 November 2004 04:25, Chris Wedgwood wrote: > If UML wants to open an xterm channel and the xterm does not run > properly (eg. terminates soon after starting) we will get a hang. > This avoids the most common cause for this and adds a comment (which > long term will go away with a rewrite of that code I guess?) Thanks for drawing attention on this! I hadn't had the time to dig this out... for now reasonably correct, but no time to review it properly now. The review would probably try to find a more direct fix to this... -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Chris W. <cw...@f0...> - 2004-11-16 06:42:03
|
On Mon, Nov 15, 2004 at 06:24:29PM +0100, Blaisorblade wrote: > Thanks for drawing attention on this! I hadn't had the time to dig > this out... for now reasonably correct it's a partial solution, in general more is required > but no time to review it properly now. The review would probably try > to find a more direct fix to this... catch the xterm process dying, up(&data->sem), and -EIO all requests from that point onwards I guess. That applies for some of the other channels too, some of the code would probably be abstracted a little and generalized I guess. --cw |
From: <bla...@ya...> - 2005-01-13 15:31:07
|
From: Chris Wedgwood <cw...@f0...> If UML wants to open an xterm channel and the xterm does not run properly (eg. terminates soon after starting) we will get a hang (a comment added in the patch explains why). This avoids the most common cause for this and adds a comment (which long term will go away with a rewrite of that code); the complete fix would be to catch the xterm process dying, up(&data->sem), and -EIO all requests from that point onwards. That applies for some of the other channels too, so part of the code should probably be abstracted a little and generalized. Signed-off-by: Chris Wedgwood <cw...@f0...> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- linux-2.6.11-paolo/arch/um/drivers/xterm.c | 7 +++++++ linux-2.6.11-paolo/arch/um/drivers/xterm_kern.c | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff -puN arch/um/drivers/xterm_kern.c~uml-xterm-clarify arch/um/drivers/xterm_kern.c --- linux-2.6.11/arch/um/drivers/xterm_kern.c~uml-xterm-clarify 2005-01-13 02:02:52.984641072 +0100 +++ linux-2.6.11-paolo/arch/um/drivers/xterm_kern.c 2005-01-13 02:02:52.988640464 +0100 @@ -46,6 +46,8 @@ int xterm_fd(int socket, int *pid_out) printk(KERN_ERR "xterm_fd : failed to allocate xterm_wait\n"); return(-ENOMEM); } + + /* This is a locked semaphore... */ *data = ((struct xterm_wait) { .sem = __SEMAPHORE_INITIALIZER(data->sem, 0), .fd = socket, @@ -55,12 +57,17 @@ int xterm_fd(int socket, int *pid_out) err = um_request_irq(XTERM_IRQ, socket, IRQ_READ, xterm_interrupt, SA_INTERRUPT | SA_SHIRQ | SA_SAMPLE_RANDOM, "xterm", data); - if(err){ + if (err){ printk(KERN_ERR "xterm_fd : failed to get IRQ for xterm, " "err = %d\n", err); ret = err; goto out; } + + /* ... so here we wait for an xterm interrupt. + * + * XXX Note, if the xterm doesn't work for some reason (eg. DISPLAY + * isn't set) this will hang... */ down(&data->sem); free_irq_by_irq_and_dev(XTERM_IRQ, data); diff -puN arch/um/drivers/xterm.c~uml-xterm-clarify arch/um/drivers/xterm.c --- linux-2.6.11/arch/um/drivers/xterm.c~uml-xterm-clarify 2005-01-13 02:02:52.985640920 +0100 +++ linux-2.6.11-paolo/arch/um/drivers/xterm.c 2005-01-13 02:02:52.988640464 +0100 @@ -97,6 +97,13 @@ int xterm_open(int input, int output, in if(os_access(argv[4], OS_ACC_X_OK) < 0) argv[4] = "port-helper"; + /* Check that DISPLAY is set, this doesn't guarantee the xterm + * will work but w/o it we can be pretty sure it won't. */ + if (!getenv("DISPLAY")) { + printk("xterm_open: $DISPLAY not set.\n"); + return -ENODEV; + } + fd = mkstemp(file); if(fd < 0){ printk("xterm_open : mkstemp failed, errno = %d\n", errno); _ |
From: <bla...@ya...> - 2005-01-13 15:31:10
|
From: Chris Wedgwood <cw...@f0...> If UML wants to open an xterm channel and the xterm does not run properly (eg. terminates soon after starting) we will get a hang (a comment added in the patch explains why). This avoids the most common cause for this and adds a comment (which long term will go away with a rewrite of that code); the complete fix would be to catch the xterm process dying, up(&data->sem), and -EIO all requests from that point onwards. That applies for some of the other channels too, so part of the code should probably be abstracted a little and generalized. Signed-off-by: Chris Wedgwood <cw...@f0...> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- linux-2.6.11-paolo/arch/um/drivers/xterm.c | 7 +++++++ linux-2.6.11-paolo/arch/um/drivers/xterm_kern.c | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff -puN arch/um/drivers/xterm_kern.c~uml-xterm-clarify arch/um/drivers/xterm_kern.c --- linux-2.6.11/arch/um/drivers/xterm_kern.c~uml-xterm-clarify 2005-01-13 02:02:52.984641072 +0100 +++ linux-2.6.11-paolo/arch/um/drivers/xterm_kern.c 2005-01-13 02:02:52.988640464 +0100 @@ -46,6 +46,8 @@ int xterm_fd(int socket, int *pid_out) printk(KERN_ERR "xterm_fd : failed to allocate xterm_wait\n"); return(-ENOMEM); } + + /* This is a locked semaphore... */ *data = ((struct xterm_wait) { .sem = __SEMAPHORE_INITIALIZER(data->sem, 0), .fd = socket, @@ -55,12 +57,17 @@ int xterm_fd(int socket, int *pid_out) err = um_request_irq(XTERM_IRQ, socket, IRQ_READ, xterm_interrupt, SA_INTERRUPT | SA_SHIRQ | SA_SAMPLE_RANDOM, "xterm", data); - if(err){ + if (err){ printk(KERN_ERR "xterm_fd : failed to get IRQ for xterm, " "err = %d\n", err); ret = err; goto out; } + + /* ... so here we wait for an xterm interrupt. + * + * XXX Note, if the xterm doesn't work for some reason (eg. DISPLAY + * isn't set) this will hang... */ down(&data->sem); free_irq_by_irq_and_dev(XTERM_IRQ, data); diff -puN arch/um/drivers/xterm.c~uml-xterm-clarify arch/um/drivers/xterm.c --- linux-2.6.11/arch/um/drivers/xterm.c~uml-xterm-clarify 2005-01-13 02:02:52.985640920 +0100 +++ linux-2.6.11-paolo/arch/um/drivers/xterm.c 2005-01-13 02:02:52.988640464 +0100 @@ -97,6 +97,13 @@ int xterm_open(int input, int output, in if(os_access(argv[4], OS_ACC_X_OK) < 0) argv[4] = "port-helper"; + /* Check that DISPLAY is set, this doesn't guarantee the xterm + * will work but w/o it we can be pretty sure it won't. */ + if (!getenv("DISPLAY")) { + printk("xterm_open: $DISPLAY not set.\n"); + return -ENODEV; + } + fd = mkstemp(file); if(fd < 0){ printk("xterm_open : mkstemp failed, errno = %d\n", errno); _ |