On Fri, Jun 24, 2011 at 03:39:51PM +0200, Stephen Shirley wrote: > On Fri, Jun 24, 2011 at 14:44, Rene Nussbaumer <[email protected]> wrote: > > On Fri, Jun 24, 2011 at 1:55 PM, Stephen Shirley <[email protected]> wrote: > >> diff --git a/tools/xm-console-wrapper b/tools/xm-console-wrapper > >> new file mode 100755 > >> index 0000000..a5085c9 > >> --- /dev/null > >> +++ b/tools/xm-console-wrapper > >> @@ -0,0 +1,33 @@ > >> +#!/bin/sh > > > > Change this to /bin/bash, we're using bash everywhere :). > > Heh, ok done. I had copied from tools/kvm-ifup. > > >> +unpause() { > >> + ispaused=$(xm list -l "$INSTANCE" 2>/dev/null | > >> + sed -n 's/^[[:blank:]]*(state ..\(.\)...)/\1/p') > >> + [ "$ispaused" == "p" ] || return > > > > Use [[ and ]] in bash. > > Also done. > > >> + # Sleep to allow xm console enough time to connect > >> + sleep 3 > > > > Somehow I dislike this, can we maybe use kill -0 or something to test > > if a signal is reachable and assume that when signal handling is done > > that the process is alive? > > There's a few problems here. The first is that 'xm console' needs to > read/write from stdin/stdout, so it has to run in the foreground. This > means that the unpause stuff runs in the background, and doesn't know > the PID of xm console. Also i can't see any way to actually eliminate > the race condition, because we do not control xm console. > > The bigger thing though is that we really just don't care. This is > intended to help with debugging of installation scripts etc, i.e. not > something for everyday use. If the machine is very heavily overloaded > and xm console hasn't already attached to the console, we'll have > missed the very beginning of the bootup, but nothing bad has happened. > If it's important to the user, they stop the instance and do it again. > To my mind the very simple (and stupid, yes) approach above covers 99% > of cases, and the 1% it doesn't cover is (a) seriously non-trivial, > and (b) going to seriously complicate stuff even trying to cover > without actually fixing it completely.
That's a reasonable (let's say :) argument, but my problem is with a different aspect. It's fine to use such methods, but they need to be _well_ documented. See for example the discussion around commit 0f7098f3dd, where a 10 second sleep was used and noone was sure anymore why it was needed, and how the value was computed. As long as it is documented, IMHO it's fine to use as such. thanks, iustin
