Re: [Xen-devel] [PATCH 1/8] Add core.sh and wrapper function

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 13 Apr 2015, George Dunlap wrote:
> On 04/10/2015 03:29 PM, Stefano Stabellini wrote:
> >> +arg_parse_cmd=\
> >> +"local -a args;
> >> +local _a;
> >> +local _vn;
> >> +local _m;
> >> +
> >> +_m=true;
> >> +
> >> +for _a in \"\$@\" ; do
> >> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
> >> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
> >> +        false && echo Parameter;
> >> +        _vn=\${_a%%=*};
> >> +        eval \"local \$_vn\";
> >> +        eval \"\$_a\";
> >> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
> >> +        false && echo Separator;
> >> +        _m=false;
> >> +    else
> >> +        false && echo Argument;
> >> +        _m=false;
> >> +        args+=(\"\$_a\");
> >> +    fi;
> >> +done"
> > 
> > I am sorry but I cannot bear myself to introduce so much complexity into
> > the world.  raisin is supposed to be easy to read.  If we end up
> > actually needing something so powerful and so complex in the future we
> > can import it then, but I think that at the moment we can do nicely
> > without it.
> 
> I can certainly understand the sentiment, but the idea behind this
> calling convention is actually to make it *easier* both to read and
> debug most of the source file, at the cost of two very complex, but
> well-tested and generic macros ($arg_parse and $requireargs).
> 
> Consider some criteria:
> - Difficulty of writing or reading code that calls a function
> - Difficulty of writing a function which will be called
> - Availability of sensible defaults
> - Ability to make sure all necessary arguments have been specified
> - Passing function arguments into sub-functions
> - Global variables are nasty to debug
>
> Consider alternate ways of passing arguments into functions:
> 
> 1. Positional
> 
> i.e., "cp SRC DEST"
> 
> In this model, it's easy to make sure that all necessary arguments have
> been specified; and there are no nasty global variables to work with.
> 
> However is really hard to program and read because you have to remember
> what argument goes where.  You also have to specify all possible
> arguments -- you can't have a default.  And if you need to take an
> argument and pass it to another function you call, you have to repeat it
> again.
> 
> 2. Option
> 
> i.e., "-v --components='a b'"
> 
> This is easier to read if you use long options (or common ones like -v
> or -y), and you can also have defaults.  Additionally, you can have a
> certain amount of error checking -- if you get an argument you don't
> recognize you can throw an error.
> 
> But you have to write a lot of boilerplate option processing at the top
> of each function; and if you have anything a bit complex (line
> "--components= above"), it makes it a lot harder to read.  And you have
> to write new code every time, giving you more opportunity for bugs.
> 
> Consider the argument parsing code you have in raise at the moment -- it
> assumes that you will run a command without any additional parameters.
> What if you wanted to do something like this:
> 
> "raise -v build xen"
> 
> The current code would have to be refactored.
> 
> And you have to write a lot of custom code to check to make sure that
> all the required arguments have been passed in.
> 
> And you have the same problem that if you need to pass an argument
> further down the call stack, you have to repeat it again.
> 
> 3. Global variables
> 
> i.e.,
> "VAR=foo
>  function"
> 
> This is a bit easier to read in the sense that you're using full
> variable names for options.  It's easier to write, since you don't have
> any parsing code.  Additionally, variables can be passed further down
> the calling stack automatically without having to be explicitly copied
> around.  In theory you should be able to have sensible local defaults.
> 
> But you have all the problems with global variables being really hard to
> debug.
> 
> 4. Prefix variable
> 
> i.e., "YES=y check-builddep"
> 
> This is a bit easier to read in the sense that you're using full
> variable names for options.  It's easier to write, since you don't have
> any parsing code.  Additionally, variables can be passed further down
> the calling stack automatically without having to be explicitly copied
> around.  In theory you should be able to have sensible local defaults.
> 
> I hadn't thought about this as a calling convention.  It certainly has
> the advantage that it's build into bash and fairly well-understood by
> experienced users.
> 
> But I do think it's a bit unnatural to have to prefix all the variables
> to the function.
> 
> 5. VAR=VAL automatic parsing with inheritance
> 
> This allows you to have a rich, consistent way to pass in arguments that
> are descriptive.  It's easy to write code to parse the arguments and
> check to make sure you have all the ones you need: Just add $arg_parse
> and $requireargs at the top of your function.  They are automatically
> declared local, so there's no worry about polluting the global
> namespace.  They are automatically inherited, so you don't have to keep
> passing information further down the stack.  And you can have sensible
> defaults.
> 
> I think #5 is well worth the little bit of macro hackery confined to a
> handful of well-tested macros, particularly if raise becomes a fairly
> large and fully-functional script.  I've been using this calling
> convention for several years now in my own scripts, so it's pretty well
> tested code-wise, feature-wise, and it hasn't become a cancer that
> spreads throughout the rest of the code. :-)


We have two different use cases:

- command line parsing in the main script, raise

- argument parsing in the other functions

I think it might be OK to have something like 5) just for the command
line parsing in raise, for the sake of giving more flexibility to the
user. Also it would be confined to one specific call site. But I am
uncertain that the pros outweigh the cons in this case.

I would rather avoid it in all the other functions. The reason is that
not only is very complex, but it makes the code quite different from
most of the other bash code out there, including DevStack.  For the
principle of least astonishment, I would rather have something more
common and easier to read and guess the meaning of it.

I would be content with requiring a simple comment on top of each new
function that documents which are the arguments of the function.
Otherwise option 4) would also work.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel