Re: [Xen-devel] [PATCH] Fix xencommons for NetBSD

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2011-07-15 at 09:59 +0100, Christoph Egger wrote:
> On 07/15/11 10:01, Ian Campbell wrote:
> > On Fri, 2011-07-15 at 08:38 +0100, Roger Pau Monné wrote:
> >> # HG changeset patch
> >> # User royger
> >> # Date 1310722656 -7200
> >> # Node ID d3b9e6f9536c84d595276f8c12735736d5593e13
> >> # Parent  8c7e3cfc33b6151fbd4fc6e1aeae132a9b1ccfa5
> >> xencommons NetBSD init script:
> >
> > In general it is useful if the first line of the commit message
> > standsalone as a summary, since hg will often display it that way (e.g.
> > in hg log, on hg web etc).
> >
> >> Clean up of xenstore database on init.
> >> Prevent xenstore from restarting.
> >> Set Domain-0 name on init.
> >>
> >> Signed-off-by: Roger Pau Monne<roger.pau@xxxxxxxxxxxxx>
> >>
> >> diff -r 8c7e3cfc33b6 -r d3b9e6f9536c tools/hotplug/NetBSD/rc.d/xencommons
> >> --- a/tools/hotplug/NetBSD/rc.d/xencommons Fri Jul 15 11:29:21 2011 +0200
> >> +++ b/tools/hotplug/NetBSD/rc.d/xencommons Fri Jul 15 11:37:36 2011 +0200
> >> @@ -23,6 +23,9 @@
> >>   XENSTORED_PIDFILE="/var/run/xenstored.pid"
> >>   XENCONSOLED_PIDFILE="/var/run/xenconsoled.pid"
> >>   XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
> >> +#XENBACKENDD_DEBUG=1
> >> +#XENCONSOLED_TRACE=1
> >> +#XENSTORED_TRACE="/var/log/xen/xenconsole-trace.log"
> >>
> >>   xen_precmd()
> >>   {
> >> @@ -33,15 +36,24 @@
> >>
> >>   xen_startcmd()
> >>   {
> >> -  printf "Starting xenservices: xenstored, xenconsoled, xenbackendd.\n"
> >> -  XENSTORED_ARGS=" --pid-file ${XENSTORED_PIDFILE}"
> >> -  if [ -n "${XENSTORED_TRACE}" ]; then
> >> -          XENSTORED_ARGS="${XENSTORED_ARGS} -T 
> >> /var/log/xen/xenstored-trace.log"
> >> +  xenstored_pid=$(check_pidfile ${XENSTORED_PIDFILE} ${SBINDIR}/xenstored)
> >> +  if test -z $xenstored_pid; then
> >> +          printf "Cleaning xenstore database.\n"
> >> +          if [ -n "${XENSTORED_ROOTDIR}" ]; then
> >> +                  XENSTORED_ROOTDIR="/var/lib/xenstored"
> >> +          fi
> >> +          rm -f ${XENSTORED_ROOTDIR}/tdb*&>/dev/null
> >> +          printf "Starting xenservices: xenstored, xenconsoled, 
> >> xenbackendd.\n"
> >> +          XENSTORED_ARGS=" --pid-file ${XENSTORED_PIDFILE}"
> >> +          if [ -n "${XENSTORED_TRACE}" ]; then
> >> +                  XENSTORED_ARGS="${XENSTORED_ARGS} -T 
> >> /var/log/xen/xenstored-trace.log"
> >> +          fi
> >> +          ${SBINDIR}/xenstored ${XENSTORED_ARGS}
> >> +          sleep 5
> >
> > sleep 5 seems a bit drastic. Perhaps steal the checking loop from the Linux 
> > xencommons?
> 
> I think, the sleep 5 can be reduced to sleep 2 since xenstored launches 
> much faster when there is no xenstored database to check.
> The sleep is there to delay the next start up script which starts xend.
> And starting xend fails when xenstored isn't ready before.

Why hardcode a "2" (which can easily be either too short or too long
depending on circumstances) when you could cut-and-paste something
better (from tools/hotplug/Linux/init.d/xencommons):
        local time=0
        local timeout=30
        ...
                # Wait for xenstored to actually come up, timing out after 30 
seconds
                while [ $time -lt $timeout ] && ! `xenstore-read -s /local 
>/dev/null 2>&1` ; do
                    echo -n .
                    time=$(($time+1))
                    sleep 1
                done

Is $(( ... )) a bashism? `expr $time + 1` instead?

Ian.


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