UML: fix missing non-blocking I/O, now DEBUG_SHIRQ works

DEBUG_SHIRQ generates spurious interrupts, triggering handlers such as mconsole_interrupt() or line_interrupt(). They expect data to be available to be read from their sockets/pipes, but in the case of spurious interrupts, the host didn't actually send anything, so UML hangs in read() and friends. Setting those fd's as O_NONBLOCK makes DEBUG_SHIRQ-enabled UML kernels boot and run correctly.

arch/um/drivers/mconsole_kern.c +4 -5

index 542c9ef..02d132e 100644
@@ -74,12 +74,11 @@ static DECLARE_WORK(mconsole_work, mc_work_proc);
 
 static irqreturn_t mconsole_interrupt(int irq, void *dev_id)
 {
-	/* long to avoid size mismatch warnings from gcc */
-	long fd;
+	int fd;
 	struct mconsole_entry *new;
 	static struct mc_request req;	/* that's OK */
 
-	fd = (long) dev_id;
+	fd = *((int *) dev_id);
 	while (mconsole_get_request(fd, &req)){
 		if(req.cmd->context == MCONSOLE_INTR)
 			(*req.cmd->handler)(&req);

@@ -798,10 +797,10 @@ void mconsole_stack(struct mc_request *req)
  */
 static char *notify_socket = NULL;
 
+static int sock;
+
 static int mconsole_init(void)
 {
-	/* long to avoid size mismatch warnings from gcc */
-	long sock;
 	int err;
 	char file[256];
 

I don't really like this section though. The casting I have now isn't pleasant, but I don't like adding a new global to get rid of it.

Quoted 34 lines

arch/um/drivers/mconsole_kern.c +4 -5

index 542c9ef..02d132e 100644
@@ -74,12 +74,11 @@ static DECLARE_WORK(mconsole_work, mc_work_proc);
 
 static irqreturn_t mconsole_interrupt(int irq, void *dev_id)
 {
-	/* long to avoid size mismatch warnings from gcc */
-	long fd;
+	int fd;
 	struct mconsole_entry *new;
 	static struct mc_request req;	/* that's OK */
 
-	fd = (long) dev_id;
+	fd = *((int *) dev_id);
 	while (mconsole_get_request(fd, &req)){
 		if(req.cmd->context == MCONSOLE_INTR)
 			(*req.cmd->handler)(&req);

@@ -798,10 +797,10 @@ void mconsole_stack(struct mc_request *req)
  */
 static char *notify_socket = NULL;
 
+static int sock;
+
 static int mconsole_init(void)
 {
-	/* long to avoid size mismatch warnings from gcc */
-	long sock;
 	int err;
 	char file[256];
 

@@ -818,7 +817,7 @@ static int mconsole_init(void)
 

arch/um/drivers/mconsole_kern.c static int mconsole_init(void) · line 823

Quoted 7 lines

arch/um/drivers/mconsole_kern.c +1 -1

 	err = um_request_irq(MCONSOLE_IRQ, sock, IRQ_READ, mconsole_interrupt,
 			     IRQF_DISABLED | IRQF_SHARED | IRQF_SAMPLE_RANDOM,
-			     "mconsole", (void *)sock);
+			     "mconsole", &sock);
 	if (err){
 		printk("Failed to get IRQ for management console\n");
 		return(1);

Jeff

No offense, but this is an ugly hack.

What if sizeof(int) != sizeof(long)?

You're calling glibc functions
with that fd as a parameter. On some arches, compiling will issue warnings or simply fail.

An alternative would be to use kmalloc instead of a global static variable. Do you like this one more?

*This message was transferred with a trial version of CommuniGate(r) Pro* *This message was transferred with a trial version of CommuniGate(r) Pro*

Quoted 1 line · Eduard-Gabriel Munteanu

No offense, but this is an ugly hack.

I'm not going to defend it too much, but the alternatives don't seem any better to me.

Quoted 1 line

What if sizeof(int) != sizeof(long)?

Doesn't matter - the casting will preserve the value. Of greater concern is the relationship between sizeof(void *) and sizeof(long). That's not guaranteed by the standard, but is by gcc.

Quoted 3 lines

You're calling glibc functions
with that fd as a parameter. On some arches, compiling will issue warnings or simply fail.

Which ones?

Quoted 1 line

An alternative would be to use kmalloc instead of a global static variable. Do you like this one more?

No, that trades the global variable for a new point of failure.

The main reason I like the current casting better than your global (not by a lot) is that globals have to be audited for SMP safety. So, I don't want any globals which don't need to be. This implies a local, and to minimize the machinery associated with that (kmalloc, or passing a pointer and synchronizing to avoid returning too soon), just passing the descriptor in the pointer and accepting the casting needed to get it through the compiler.

Jeff

The cast isn't done right. Doing "fd = (long) dev_id;" doesn't help, since you pass fd to mconsole_get_request() as is. And mconsole_get_request() expects an integer:
int mconsole_get_request(int fd, struct mc_request *req)

This will generate at least a warning on arches where sizeof(int) != sizeof(long).

And by the way, AFAIK, GCC has the habit of breaking compatibility with some userspace apps when a new GCC major version is released. And, AFAIK, they're moving towards standards, so relying on GCC's "guarantees" may backfire.

Quoted 6 lines

Quoted 3 lines

You're calling glibc functions
with that fd as a parameter. On some arches, compiling will issue warnings or simply fail.

Which ones?

An example is sparc64:
quote from

Quoted 1 line

Quoted 1 line

http://lxr.linux.no/source/include/asm-sparc64/types.h#L49

Really, a kmalloc() isn't such a big deal, it only happens once and we're not in interrupt context.

One the other hand, ensuring safety and
portability on other arches is something that needs to be taken care of.

No it won't. UML builds without warnings here on x86_64.

Okay, I don't have an x86_64, sparc64 or something similar, as my computer is an x86, so I can't contradict this. If everything is fine on such arches, no fix is needed when nothing's broken... though I still think it's kind of ugly (though it's somehow ingenious as a hack) :)

Do you want me to resubmit the patch without these changes? I'll be back in a few hours (got to go now) and trim this out from the patch if that's necessary.