[sheepdog] [PATCH 1/4] tests/fucntional: add a function for invoking sheep with zone
Liu Yuan
namei.unix at gmail.com
Sat Jan 11 15:00:20 CET 2014
On Sat, Jan 11, 2014 at 10:49:11PM +0900, Hitoshi Mitake wrote:
> At Sat, 11 Jan 2014 21:39:23 +0800,
> Liu Yuan wrote:
> >
> > On Sat, Jan 11, 2014 at 10:28:33PM +0900, Hitoshi Mitake wrote:
> > > At Sat, 11 Jan 2014 21:07:18 +0800,
> > > Liu Yuan wrote:
> > > >
> > > > On Sat, Jan 11, 2014 at 09:51:36PM +0900, Hitoshi Mitake wrote:
> > > > > 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.
> > > >
> > > > It is ugly to add yet another helper if we can abstract it out in a single helper
> > > > without many lines of code. What do you mean by 'confusing'? Describe your reasons
> > > > directly instead of only using a abstract adjective, which speak nothing useful
> > > > at all.
> > >
> > > I already described the reason: The scheme cannot handle other options
> > > which have string parameters at all.
> > >
> > > Parsing single string parameter and detect which options are
> > > overwritten is a very bad approach. Because a parameter of an option
> > > can can be a string which can contain other option. e.g. if we
> > > overwrite the option -l with this scheme, this scheme crashes. Because
> > > -l can accept a parameter like dir=<path>, and <path> can be
> > > "a-z". And the parameter breaks the scheme. Of course the "a-z" is a
> > > really artificial example. But it is a valid parameter of -l so we
> > > have to handle it.
> >
> > We don't have demind to overwrite a string-based parameter. We are not going
> > to write a generic _start_sheep() which will handle all the paramter tricks,
> > instead we actually write a _start_sheep() tailored to our needs.
>
> If the approach is not generic, changing _start_sheep() isn't
> worthful. If we need to handle an option which have string parameters
> like above "-l", we need to use other data structure. And it would
> require to change caller site. So the approach will be very shot
> living thing and ugly.
I said it already that we won't deal with '-l' or '-w' or any other string based
parameters for NOW. Checking '-z' option *solves* the problem NOW.
>
> >
> > Your current demind is to pass '-z' option to _start_sheep(), I think overwrite
> > it in _start_sheep is a feasible approach and keep _start_sheep as elegent
> > as before.
>
> Please show me evidence of "feasible". Describe your reasons directly
> instead of an abstract adjective.
I think I already wrote it concretely in my previous mails and I copied it again
_start_sheep()
{
check if '-z' is specified by user in $2, if not, pass "-z $1"
...
}
Then _start_sheep 1 "-z 1" will serve your demind well.
> >
> > > So if we want to detect overwritten options by the parameter of
> > > _sheep_start(), we need to use other data structure than string.
> > >
> > > Of course, adding helper functions is not an elegant approach. But the
> > > approach of parsing parameter is very half-baked and ill-served. It
> > > cannot handle options which can have string parameters.
> >
> > Unless we have a relistic demind that require us to break _start_sheep into
> > several helpers, we should try our best to keep a single helper. The future
> > *undefined* demind shoudn't play a role for design.
>
> As I described in the above, the future undefined demand will play a
> role for design.
No, '--log' should always be debug mode. I can't see a real demind to deal with
it like '-z'. Even in the future it is true, let's handle it in the future, which
is in a better position than NOW.
As a final suggestion, adding every helper for every option should be avoided as
much as possible. Current demind to handle '-z' isn't a compelling reason to
break it into several handlers and especially making the paramter lenthy than
conventionly perceived two paramter list.
Thanks
Yuan
More information about the sheepdog
mailing list