[PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Harry Butterworth]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

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.

Signed-off-by: Harry Butterworth <butterwo@uk.ibm.com>

[-- Attachment #2: daemonize-blktapctrl.patch --]
[-- Type: text/x-patch, Size: 1270 bytes --]

diff -r d2bf1a7cc131 -r 4dec6098b26f tools/blktap/drivers/blktapctrl.c
--- a/tools/blktap/drivers/blktapctrl.c	Sat Jul 29 13:05:59 2006
+++ b/tools/blktap/drivers/blktapctrl.c	Mon Jul 31 15:07:56 2006
@@ -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();

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Muli Ben-Yehuda]

On Mon, Jul 31, 2006 at 04:15:42PM +0100, Harry Butterworth wrote:

> +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).

> +	if (pid != 0)
> +		exit(0);

If fork() failed, this will cause us to exit(0) which doesn't seem
particularly appropriate.

> +	/* Session leader so ^C doesn't whack us. */
> +	setsid();

In theory setsid() can fail.

> +	/* 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.
> +
> +#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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Harry Butterworth]

On Mon, 2006-07-31 at 20:31 +0300, Muli Ben-Yehuda wrote:
> On Mon, Jul 31, 2006 at 04:15:42PM +0100, Harry Butterworth wrote:
> 
> > +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).
> 
> > +	if (pid != 0)
> > +		exit(0);
> 
> If fork() failed, this will cause us to exit(0) which doesn't seem
> particularly appropriate.
> 
> > +	/* Session leader so ^C doesn't whack us. */
> > +	setsid();
> 
> In theory setsid() can fail.
> 
> > +	/* 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.
> > +
> > +#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.

> > +
> > +        close(STDIN_FILENO);
> 
> Mixed tabs and spaces, ugh.

My mistake.

>  Also, fileno(stdin) is nicer.

Why? I don't think so.

> 
> Cheers,
> Muli

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Muli Ben-Yehuda]

On Tue, Aug 01, 2006 at 11:01:36AM +0100, Harry Butterworth wrote:

> >  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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Keir Fraser]

On 1 Aug 2006, at 11:06, Muli Ben-Yehuda wrote:

>>>  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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Muli Ben-Yehuda]

On Tue, Aug 01, 2006 at 11:13:29AM +0100, Keir Fraser wrote:
> 
> On 1 Aug 2006, at 11:06, Muli Ben-Yehuda wrote:
> 
> >>> 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).

Does it make sense for this to be folded into a common utility
library?

Cheers,
Muli

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Harry Butterworth]

OK.  I'll pull and redo both using daemon() if that's OK.

On Tue, 2006-08-01 at 11:13 +0100, Keir Fraser wrote:
> On 1 Aug 2006, at 11:06, Muli Ben-Yehuda wrote:
> 
> >>>  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
> 
> 

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Harry Butterworth]

On Tue, 2006-08-01 at 13:18 +0300, Muli Ben-Yehuda wrote:
> On Tue, Aug 01, 2006 at 11:13:29AM +0100, Keir Fraser wrote:
> > 
> > On 1 Aug 2006, at 11:06, Muli Ben-Yehuda wrote:
> > 
> > >>> 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).
> 
> Does it make sense for this to be folded into a common utility
> library?

Anil Madhavapeddy pointed out it already is: man 3 daemon.

> 
> Cheers,
> Muli
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Keir Fraser]

On 1 Aug 2006, at 11:18, Muli Ben-Yehuda wrote:

>> 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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Keir Fraser]

On 1 Aug 2006, at 11:19, Harry Butterworth wrote:

> 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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Muli Ben-Yehuda]

On Tue, Aug 01, 2006 at 11:20:59AM +0100, Harry Butterworth wrote:

> > 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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Keir Fraser]

On 1 Aug 2006, at 11:27, Muli Ben-Yehuda wrote:

>>> 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

Re: [PATCH] Fix bug #709 by daemonizing blktapctrl and closing stdin, stdout and stderr

[Jimi Xenidis]

On Aug 1, 2006, at 6:33 AM, Keir Fraser wrote:

>
> On 1 Aug 2006, at 11:27, Muli Ben-Yehuda wrote:
>
>>>> 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.

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