[sheepdog] [PATCH 1/4] tests/fucntional: add a function for invoking sheep with zone

[Hitoshi Mitake]

At Sat, 11 Jan 2014 19:56:14 +0800,
Liu Yuan wrote:
> 
> On Sat, Jan 11, 2014 at 08:49:00PM +0900, Hitoshi Mitake wrote:
> > At Sat, 11 Jan 2014 19:26:05 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Sat, Jan 11, 2014 at 08:09:16PM +0900, Hitoshi Mitake wrote:
> > > > At Sat, 11 Jan 2014 18:34:54 +0900,
> > > > Hitoshi Mitake wrote:
> > > > > 
> > > > > At Fri, 10 Jan 2014 18:00:14 +0800,
> > > > > Liu Yuan wrote:
> > > > > > 
> > > > > > On Fri, Jan 10, 2014 at 03:20:42PM +0900, Hitoshi Mitake wrote:
> > > > > > > Reported-by: Marcin Mirosław <marcin at mejor.pl>
> > > > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > > > > ---
> > > > > > >  tests/functional/common.rc |   14 ++++++++++++--
> > > > > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/functional/common.rc b/tests/functional/common.rc
> > > > > > > index 3785bae..f1ee432 100644
> > > > > > > --- a/tests/functional/common.rc
> > > > > > > +++ b/tests/functional/common.rc
> > > > > > > @@ -282,7 +282,7 @@ _valgrind_dog()
> > > > > > >      return $ret
> > > > > > >  }
> > > > > > >  
> > > > > > > -_start_sheep()
> > > > > > > +_do_start_sheep()
> > > > > > >  {
> > > > > > >      # ensure that sheep is not running
> > > > > > >      local running=true
> > > > > > > @@ -304,13 +304,23 @@ _start_sheep()
> > > > > > >  	MD_STORE=",$STORE/$1/d0,$STORE/$1/d1,$STORE/$1/d2"
> > > > > > >      fi
> > > > > > >  
> > > > > > > -    $SHEEP $STORE/$1$MD_STORE -z $1 -p $((7000+$1)) -c $DRIVER $SHEEP_OPTIONS $2
> > > > > > > +    $SHEEP $STORE/$1$MD_STORE -z $2 -p $((7000+$1)) -c $DRIVER $SHEEP_OPTIONS $3
> > > > > > >  
> > > > > > >      if [ $? != 0 ]; then
> > > > > > >          _die "cannot start sheep $1"
> > > > > > >      fi
> > > > > > >  }
> > > > > > >  
> > > > > > > +_start_sheep()
> > > > > > > +{
> > > > > > > +    _do_start_sheep $1 $1 $2
> > > > > > > +}
> > > > > > > +
> > > > > > > +_start_sheep_with_zone()
> > > > > > > +{
> > > > > > > +    _do_start_sheep $1 $2 $3
> > > > > > > +}
> > > > > > > +
> > > > > > 
> > > > > > I think it would be better to add a more generic helper such as
> > > > > > 
> > > > > > _start_sheep_raw()
> > > > > > {
> > > > > > 	$SHEEP $STORE/$1$MD_STORE -p $((7000+$1)) -c $DRIVER $SHEEP_OPTIONS $2
> > > > > > }
> > > > > > 
> > > > > > which allow more flexity, which we can
> > > > > > 
> > > > > > _start_sheep_raw 1 "-z 1" which explicitly pass options as parameters and no
> > > > > > need to add burden to users that what is parameter list for this helper.
> > > > > 
> > > > > I agree. I'll implement it in v2.
> > > > 
> > > > On the second thought, _start_sheep_raw() seems to not a good
> > > > approach. If we employ this approach, preparing default values of
> > > > every parameter is required. And we need to replace the default values
> > > > with passed ones. It requires needless complexity and I think the zone
> > > > specific wrapper can be acceptable.
> > > > 
> > > 
> > > I don't think so. What you want is to add '-z x' to _start_sheep and not affect
> > > old script. _start_sheep() already well perceived with first one as sheep
> > > identity and then apending user-defined parameters as string. Based on this
> > > convention, start a sheep with a specified zone should be:
> > > 
> > >   _start_sheep 1 "-z 1"
> > > 
> > > I think we can achieve this objective by modifying _start_sheep as following
> > > 
> > > _start_sheep()
> > > {
> > >     ...
> > >     if '-z' not specified in the $2, then -z $1	
> > 
> > If we need to pass other options explicitly in our test scripts, it
> > would be required. But zone is the only one option which needs be
> > passed to sheep explicitly.
> 
> sometimes we pass '-w -j -i' options, which current _start_sheep() handle well
> because they don't need default value at all.
> 
> > 
> > BTW, if we write such a generalized _start_sheep(), passing
> > overwritten options in one string is not enough. e.g. other
> > options like -w and -j can have the string "-z" in their
> > parameters. We need to use dictionaly or something else. Do we really
> > need such a functional _start_sheep() now?
> > 
> 
> I think no. Let's keep it as simple as possible. Curently only '-z' option has
> the possibility to be overwritten by parameters, so let's simply check '-z' only
> Unless we have many options need to be overriten, we don't need to introduce a
> complex data structure for _start_sheep.

If we only need to handle -z in the above scheme, we shouldn't
implement it. If we implement it, it will be confusing thing in the
future. The scheme cannot handle other options which have string
parameters at all. It is very half-baked. Adding a dedicated function
is far better.

Thanks,
Hitoshi