Balazs Scheidler <bazsi77@gmail.com> writes:
Having now actually reviewed the code in question, I'd have some remarks:
1) template functions should rather use command-line parsing than positional arguments. (e.g. I'd prefer --method, --length, etc as arguments instead of hardcoding the order)
Well, in general, I would agree, but not in this case: for $(hash), the method is mandatory - at least in the version I made from Peter's original -, so is the string, length is the only optional argument, and $(hash --method SHA1 --length 8 $BLAH) is longer than $(hash SHA1 $BLAH 8) and is not easier to understand, either, as far as I'm concerned. When there are many more optional arguments, then --foo is superior indeed. But when there's only one... not so much. (substr uses positional arguments too, so do many other template functions, format-json being the most visible exception as far as I see, and due to value-pairs, that has a lot of optional arguments) I'll see if I can prepare a patch that adds command-line parsing, though. I won't collapse that into a single commit, however, in hopes I can persuade you to keep the current syntax :)
2) also, it should support a series of positional arguments, which in turn should be concatenated when producing the hash
$(hash SHA1 "$HOST $PROGRAM") would already do the right thing, no? But agreed, it's less suprising if it works like you described. I already pushed out a feature/3.4/cryptofuncs branch, but I'll push a patch on top Soon(tm) that implements the automatic concatenation too. I might end up rebasing that branch.
3) basicfuncs shouldn't link to the crypto library, rather a new cryptofuncs module should be introduced, that links against libsyslog-ng-crypto.so, that module can then include the $(uuid) function too.
Done, patch on my feature/3.4/cryptofuncs branch. I had to move tf_parse_int() out of basicfuncs, into lib/misc.c, so that both basicfuncs and cryptofuncs can make use of it. And tfuuid was merged into this new module, of course. -- |8]