On Sun, 2012-08-05 at 17:37 +0200, Gergely Nagy wrote:
Gergely Nagy <algernon@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