On Sat, Jan 11, 2014 at 11:32:07PM +0900, Hitoshi Mitake wrote: > At Sat, 11 Jan 2014 22:00:20 +0800, > Liu Yuan wrote: > > > > 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. > > I noticed that sheep prioritizes latter options. e.g. if we invoke > sheep with the below command: > $ sheep -z 0 -z 1 > the sheep process will run with zone 1. > > I think simply passing "-z <zone>" as the second parameter is > enough for overwriting. If you agree with this way, I'll send v3 of my > patch. > Sure, this is the simplest approach and *best* to solve '-z' option problem. Thanks Yuan |