Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr
PATCH 1/1 Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr
This patch fixes bug #709. I've tested it by running xm-test in both HVM and non-HVM mode. xm-test no longer hangs in the enforce-dom0-cpus test.
I just copied the daemonize code from xenstored and added the lines to close the filedescriptors. This is a best effort without really 100% understanding exactly what blktapctrl is doing.
Please review carefully before applying.
tools/blktap/drivers/blktapctrl.c +34 -0
@@ -622,6 +622,39 @@DPRINTF ("Found driver: [%s]\n" ,dtypes[i]->name); } +/* Stevens. */ +static void daemonize (void ) +{ +pid_t pid; + +/* Separate from our parent via fork, so init inherits us. */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" ); +if (pid !=0 ) +exit (0 ); + +/* Session leader so ^C doesn't whack us. */ +setsid (); + +/* Let session leader exit so child cannot regain CTTY */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" ); +if (pid !=0 ) +exit (0 ); + +#ifndef TESTING /* Relative paths for socket names */ +/* Move off any mount points we might be in. */ +if (chdir ("/" ) ==-1 ) +DPRINTF ("Failed to chdir\n" ); +#endif +/* Discard our parent's old-fashioned umask prejudices. */ +umask (0 ); + +close (STDIN_FILENO ); +close (STDOUT_FILENO ); +close (STDERR_FILENO ); +} +int main (int argc,char *argv[]) {char *devname;
@@ -633,6 +666,7 @@__init_blkif ();openlog ("BLKTAPCTRL" ,LOG_CONS |LOG_ODELAY ,LOG_DAEMON ); +daemonize ();print_drivers ();init_driver_list ();
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
tools/blktap/drivers/blktapctrl.c line 632
Quoted 7 lines · Harry Butterworth
tools/blktap/drivers/blktapctrl.c +7
-0+static void daemonize (void ) +{ +pid_t pid; + +/* Separate from our parent via fork, so init inherits us. */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" );
It will be useful to know why fork() failed (i.e., print errno directly or use sterror_r() and friends).
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (pid != 0) + exit(0);
If fork() failed, this will cause us to exit(0) which doesn't seem particularly appropriate.
tools/blktap/drivers/blktapctrl.c line 637
Quoted 2 lines
tools/blktap/drivers/blktapctrl.c +2
-0+/* Session leader so ^C doesn't whack us. */ +setsid ();
In theory setsid() can fail.
tools/blktap/drivers/blktapctrl.c line 643
Quoted 5 lines
tools/blktap/drivers/blktapctrl.c +5
-0+/* Let session leader exit so child cannot regain CTTY */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" ); +if (pid !=0 ) +exit (0 );
Same comment as above.
tools/blktap/drivers/blktapctrl.c line 653
Quoted 10 lines
tools/blktap/drivers/blktapctrl.c +10
-0+ +#ifndef TESTING /* Relative paths for socket names */ +/* Move off any mount points we might be in. */ +if (chdir ("/" ) ==-1 ) +DPRINTF ("Failed to chdir\n" ); +#endif +/* Discard our parent's old-fashioned umask prejudices. */ +umask (0 ); + +close (STDIN_FILENO );
Mixed tabs and spaces, ugh. Also, fileno(stdin) is nicer.
Cheers,
Muli
Quoted 31 lines · Muli Ben-Yehuda
tools/blktap/drivers/blktapctrl.c line 632
Quoted 7 lines · Harry Butterworth
tools/blktap/drivers/blktapctrl.c +7
-0+static void daemonize (void ) +{ +pid_t pid; + +/* Separate from our parent via fork, so init inherits us. */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" );It will be useful to know why
fork()failed (i.e., print errno directly or usesterror_r()and friends).Review
Quoted 2 lines
(quoted diff) +2
-0+ if (pid != 0) + exit(0);If
fork()failed, this will cause us to exit(0) which doesn't seem particularly appropriate.tools/blktap/drivers/blktapctrl.c line 637
Quoted 2 lines
tools/blktap/drivers/blktapctrl.c +2
-0+/* Session leader so ^C doesn't whack us. */ +setsid ();In theory
setsid()can fail.tools/blktap/drivers/blktapctrl.c line 643
Quoted 5 lines
tools/blktap/drivers/blktapctrl.c +5
-0+/* Let session leader exit so child cannot regain CTTY */ +if ((pid =fork ()) <0 ) +DPRINTF ("Failed to fork daemon\n" ); +if (pid !=0 ) +exit (0 );Same comment as above.
Quoted 8 lines
tools/blktap/drivers/blktapctrl.c line 651
Quoted 8 lines
tools/blktap/drivers/blktapctrl.c +8
-0+ +#ifndef TESTING /* Relative paths for socket names */ +/* Move off any mount points we might be in. */ +if (chdir ("/" ) ==-1 ) +DPRINTF ("Failed to chdir\n" ); +#endif +/* Discard our parent's old-fashioned umask prejudices. */ +umask (0 );
OK so when I copied the code from xenstored_core.c I failed to notice that barf_perror exits with an error code and DPRINTF which I replaced it with doesn't.
That was pretty lame.
I'll fix these.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + close(STDIN_FILENO);Mixed tabs and spaces, ugh.
My mistake.
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
Quoted 3 lines
Cheers,
Muli
Quoted 3 lines · Harry Butterworth
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
fclose(stdin); /* and the rest */
fd = open(...); /* fd is 0 */close(STDIN_FILENO); /* does the wrong thing */
close(fileno(stdin)); /* does the right thing */
Cheers,
Muli
Quoted 8 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
fclose(stdin); /* and the rest */
fd = open(...); /* fd is 0 */close(STDIN_FILENO); /* does the wrong thing */
close(fileno(stdin)); /* does the right thing */
Yes, that's the better way even though we won't have done tricks such as the above when daemonise runs. Also a new patch needs to be based at least on xen-unstable 10876 which is the original patch from Harry -- it isn't in the public tree yet for some reason: I'll check up on that. And any changes may also need to be applied to the xenstored original (no sense in updating one and not the other).
Cheers,
Keir
Quoted 12 lines · Keir Fraser
Quoted 8 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
fclose(stdin); /* and the rest */
fd = open(...); /* fd is 0 */close(STDIN_FILENO); /* does the wrong thing */
close(fileno(stdin)); /* does the right thing */
Quoted 6 lines
Yes, that's the better way even though we won't have done tricks such as the above when daemonise runs. Also a new patch needs to be based at least on xen-unstable 10876 which is the original patch from Harry -- it isn't in the public tree yet for some reason: I'll check up on that. And any changes may also need to be applied to the xenstored original (no sense in updating one and not the other).
Does it make sense for this to be folded into a common utility library?
Cheers,
Muli
Quoted 20 lines · Muli Ben-Yehuda
Quoted 12 lines · Keir Fraser
Quoted 8 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
fclose(stdin); /* and the rest */
fd = open(...); /* fd is 0 */close(STDIN_FILENO); /* does the wrong thing */
close(fileno(stdin)); /* does the right thing */Quoted 6 lines
Yes, that's the better way even though we won't have done tricks such as the above when daemonise runs. Also a new patch needs to be based at least on xen-unstable 10876 which is the original patch from Harry -- it isn't in the public tree yet for some reason: I'll check up on that. And any changes may also need to be applied to the xenstored original (no sense in updating one and not the other).
Quoted 2 lines
Does it make sense for this to be folded into a common utility library?
Anil Madhavapeddy pointed out it already is: man 3 daemon.
Quoted 9 lines
Cheers,
Muli_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 4 lines · Harry Butterworth
Quoted 2 lines
Does it make sense for this to be folded into a common utility library?
Anil Madhavapeddy pointed out it already is: man 3 daemon.
Cool! I wasn't aware of it. How portable is it? how portable should the tools be in general?
Cheers,
Muli
Quoted 7 lines · Muli Ben-Yehuda
Quoted 4 lines
Quoted 2 lines
Does it make sense for this to be folded into a common utility library?
Anil Madhavapeddy pointed out it already is: man 3 daemon.
Cool! I wasn't aware of it. How portable is it? how portable should the tools be in general?
Ah, that might be a concern, for the Solaris port in particular. daemon() is I think a BSD function ported to Linux. It's not POSIX. Keeping the existing code is uglier but probably more portable.
-- Keir
Quoted 6 lines · Keir Fraser
Quoted 3 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 2 lines
Does it make sense for this to be folded into a common utility library?
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Anil Madhavapeddy pointed out it already is: man 3 daemon.
Cool! I wasn't aware of it. How portable is it? how portable should the tools be in general?
Ah, that might be a concern, for the Solaris port in particular.
daemon()is I think a BSD function ported to Linux. It's not POSIX. Keeping the existing code is uglier but probably more portable.
IMHO, it is better to use a system provided feature that has a large peer review audience than roll our own.
There are plenty of portable projects that use this method and fall back to a compatibility library for daemon(3), the OpenSSH project in particular has had to deal with it, I suggest if Solaris needs to borrow any code they borrow it from sshd.
Please also remember that daemons, since they are typically started from the craziest init scripts, should close _all_ file descriptors not explicitly opened or required by the daemon.
for (i = 0; i < sysconf(_SC_OPEN_MAX); i++) {
if (i not explicit)
close(i);
}-JX
Quoted 9 lines · Muli Ben-Yehuda
Quoted 6 lines
Yes, that's the better way even though we won't have done tricks such as the above when daemonise runs. Also a new patch needs to be based at least on xen-unstable 10876 which is the original patch from Harry -- it isn't in the public tree yet for some reason: I'll check up on that. And any changes may also need to be applied to the xenstored original (no sense in updating one and not the other).
Does it make sense for this to be folded into a common utility library?
I don't think so for just this one function. There's no obvious existing place to put it.
-- Keir
OK. I'll pull and redo both using daemon() if that's OK.
Quoted 22 lines · Keir Fraser
Quoted 8 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 1 line
Also, fileno(stdin) is nicer.
Why? I don't think so.
fclose(stdin); /* and the rest */
fd = open(...); /* fd is 0 */close(STDIN_FILENO); /* does the wrong thing */
close(fileno(stdin)); /* does the right thing */Yes, that's the better way even though we won't have done tricks such as the above when daemonise runs. Also a new patch needs to be based at least on xen-unstable 10876 which is the original patch from Harry -- it isn't in the public tree yet for some reason: I'll check up on that. And any changes may also need to be applied to the xenstored original (no sense in updating one and not the other).
Cheers,
Keir
Quoted 1 line · Harry Butterworth
OK. I'll pull and redo both using
daemon()if that's OK.
That would be okay, if we fully understand the semantics of daemon(). Do you still need to setsid() and do the second fork() if you use daemon()?
-- Keir