Xen devel - Jul 2006 - [PATCH] 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.

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


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

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

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

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


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

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

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

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


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

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

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

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

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

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
> 

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

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


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

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


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

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

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

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


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

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

  
  

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