On Mon, Sep 26, 2011 at 04:42:33PM +0200, Peter Krempa wrote: > On some systems init scripts are installed along with upstart . This may > cause trouble if user tries to restart/stop a instance of libvirtd > managed with upstart with init script. > > Upstart config file uses "respawn" stanza to ensure that libvirtd is > restarted after a crash/kill. This combined with a > > The SysV init script is now able to detect libvirtd managed with upstart > explicitly and notify the user about this. This patch also modifies the > way the PID file is handled. Libvirtd alone removes the pid file on a > successful exit, so now, the init script does not delete it. It's only > deleted while starting libvirtd, when the script detects, that no > libvirtd with pid specified in the pid file is running. > > This patch also modifies the upstart configuration file for libvirt. > Same logic as in the SysV script for handling pid files is used. The > upstart script does not explicitly check for a SysV managed instance. > Upstart alone prohibits to stop a not-started instance, so this issue is > handled automaticaly. > > https://bugzilla.redhat.com/show_bug.cgi?id=728153 > --- > initctl_check() in the SysV init file is not strongly required. The purpose > of this is to notify the user of the source of problem the user is experiencing. > > It can be left out, but then no (reasonable) notification will be provided > and the user might kill libvirtd managed by upstart (which will thereafter > respawn) > > daemon/libvirtd.init.in | 43 +++++++++++++++++++++++++++++++++++++++++-- > daemon/libvirtd.upstart | 31 ++++++++++++++++++++++++------- > 2 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in > index 0697a2b..28801d1 100644 > --- a/daemon/libvirtd.init.in > +++ b/daemon/libvirtd.init.in > @@ -43,6 +43,8 @@ LIBVIRTD_CONFIG= > LIBVIRTD_ARGS= > KRB5_KTNAME=/etc/libvirt/krb5.tab > > +INITCTL_PATH=/sbin/initctl > + > test -f @sysconfdir@/sysconfig/libvirtd && . @sysconfdir@/sysconfig/libvirtd > > export QEMU_AUDIO_DRV > @@ -56,8 +58,45 @@ fi > > RETVAL=0 > > +# Check if libvirt is managed by upstart and fail if it's the case > +initctl_check() { > + if [ -x $INITCTL_PATH ]; then > + #extract status from upstart > + LIBVIRTD_UPSTART_STATUS=`$INITCTL_PATH status libvirtd | tr "/" " " | cut -d " " -f 2` > + if [ $LIBVIRTD_UPSTART_STATUS = "start" ]; then > + logger -t "libvirtd" -s "libvirtd is managed by upstart and started, use initctl instead" > + exit 1 > + fi > + fi > +} IMHO this has no business being here. > + > +# test if a pidfile exists and if there's a libvirtd process associated with it > +pidfile_check() { > + #check if libvirtd is running > + if [ -f "$PIDFILE" ]; then > + PID=`cat $PIDFILE` > + if [ -n "$PID" ]; then > + PROCESSES=`pidof $PROCESS | grep $PID` > + if [ -n "$PROCESSES" ]; then > + logger -t "libvirtd" -s "$SERVICE with pid $PID is running"; > + exit 1 > + else > + # pidfile exists but no running libvirtd found > + # remove stuck pidfile > + rm -f $PIDFILE > + fi > + else > + # pidfile is empty > + rm -f $PIDFILE > + fi > + fi > +} This code is all bogus as of commit c8a3a265135a0527b46aeb0ebd39de8a03189fb0 Author: Daniel P. Berrange <berrange@xxxxxxxxxx> Date: Fri Aug 5 15:11:11 2011 +0100 Convert libvirtd to use crash-safe pidfile APIs With this commit, there is no such thing as a stale pidfile anymore, since we use a fcntl() lock for exclusivity, instead of merely the existance of the pidfile on disk. In fact doing an 'rm -f' on the pidfile here is actively harmful because it can delete a pidfile that another process is in the middle of creating. > diff --git a/daemon/libvirtd.upstart b/daemon/libvirtd.upstart > index fd1d951..c506d45 100644 > --- a/daemon/libvirtd.upstart > +++ b/daemon/libvirtd.upstart > @@ -7,6 +7,29 @@ stop on runlevel [!345] > > respawn > > +pre-start script > + PIDFILE=/var/run/libvirtd.pid > + #check if libvirtd is running > + if [ -f "$PIDFILE" ]; then > + PID=`cat $PIDFILE` > + if [ -n "$PID" ]; then > + PROCESSES=`pidof libvirtd | grep $PID` > + > + if [ -n "$PROCESSES" ]; then > + logger -t "libvirtd" -s "error: libvirtd is already running with pid $PID" > + stop > + exit 0 > + else > + # remove stuck pidfile > + rm -f $PIDFILE > + fi > + else > + # empty pidfile > + rm -f $PIDFILE > + fi > + fi > +end script This is all bogus for the same reason as above. > @@ -31,16 +54,10 @@ script > ulimit -c "$DAEMON_COREFILE_LIMIT" > fi > > - # Clean up a pidfile that might be left around > - rm -f /var/run/libvirtd.pid This is correct to remove though. > - > + # No pid file and/or libvirtd not running. > mkdir -p /var/cache/libvirt > rm -rf /var/cache/libvirt/* > > exec /usr/sbin/libvirtd $LIBVIRTD_CONFIG_ARGS $LIBVIRTD_ARGS > end script > > -post-stop script > - rm -f $PIDFILE As is this Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list