[syslog-ng] [PATCH] tfhash: New template function to calculate the SHA1 of its input

[Balazs Scheidler]

On Sun, 2012-08-05 at 17:37 +0200, Gergely Nagy wrote:
> Gergely Nagy <algernon at madhouse-project.org> writes:
> 
> >> 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.
> 
> Nevermind that. If we want to support multiple positional arguments,
> then --length needs to be a dashed-option anyway. I'd keep the rest (ie,
> method) as positional though.
> 
> I'll push the updated code to the same branch in a couple of hours.
> 

Thanks, another review, another set of comments (seems I see things
iteratively, larger stuff first, smaller stuff later, sorry about that).

0) argument parsing is still using positional arguments

let's discuss this IRL which form to use.

1) parse_int()

we do have a scan_int() function in str-format.c already, isn't that
enough?

I'd love to get rid of duplicated functions if possible.

2) right now cryptofuncs is linked against OPENSSL, so there's really no
point in using #ifdef ENABLE_SSL there, is there?

we might as well compile cryptofuncs conditionally, if openssl is
present do it, otherwise don't. The drawback seems to be that the
$(uuid) was available until now with libuuid too.

this is just a decision, I'm fine by a "thin" cryptofuncs in case
openssl was not found, but the original naming came because I figured
these would depend on openssl. I simply forgot the libuuid based
implementation.

3) digest naming and hash length

by using the EVP_MD API in openssl, you might get rid of the if-else-if
construct on the hash name, you'd get all openssl supported algorithms
for free.

Also, by using argv[0] to help parsing, we might have a single function
that implements all $(hash) and the actual hashing names. Certainly this
function would have to be registered under a number of names, just like
symlinks that point to the same executable in the filesystem.

4) longer term I'd split source code into separate files if there'd be
more topics than there's now, just like it is done with the basicfuncs
module to aid maintenance.

there not all modules are separate compilation units, but still make it
simple to maintain the sources.

Let me know if you want me to change these or you can finish this up.
Timewise it'd be best if you could do it, but I know I can be pesky at
times. :)

-- 
Bazsi
-- 
Bazsi