Hi Alvaro, Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera: > Hello > > I was passing by and I noticed that this needs badly pgindent, and > some > comments are enumerations that would lose formatting once through it. > For example, this would happen which is not good: > > /* > - * 1. Start digest A > - * 2. Add the password string to digest A > - * 3. Add the salt to digest A > + * 1. Start digest A 2. Add the password string to digest A 3. > Add the > + * salt to digest A > */
I didn't think about testing pg_ident, sorry. > > I suggest you can fix this by adding a "-" sign to the opening "/*" > line > so that pgindent doesn't mangle it (so it becomes /*- ). This > appears > in several places. Will try. > > It's been said in my presence that pgcrypto is obsolete and shouldn't > be > used anymore. I'm not sure I believe that, but even if that's true, > it's clear that there's plenty of people who has an interest on it, > so I > don't see that as an objection to reject this work. So let's move > on. > Oh, that's news to me. Is there a plan for it somewhere? I agree that pgcrypto is widley used and needs a proper replacement when we decide to deprecate it. > > Your test data (crypt-shacrypt.sql) looks a bit short. I noticed > that > Drepper's SHA-crypt.txt file has a bunch of test lines (starting with > the "Test vectors from FIPS 180-2: appendix B.1." comment line, as > well > as "appendix C.1" at the bottom) which perhaps could be incorporated > into the .sql script, to ensure correctness (or at least, > bug-compatibility with the reference implementation). I'd also add a > note that Drepper's implementation is public domain in crypt-sha.c's > license block. Good idea. Will do. > > I think the "ascii_dollar" variable is a bit useless. Why not just > use the > literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding > a > magic value there)? Also, I wonder if it would be better to use a > StringInfo instead of a fixed-size buffer, which would probably make > some string manipulations easier ... Or maybe not, but let's not mix > strlcat calls with strncat calls with no comments about why. > I am going to give it a try, probably helps to get rid of the strncat()/strlcat() mess. I originally thought about StringInfo but went with just the fixed length character buffers since we access them directly anyways (and the px_*/OpenSSL API needs char * ). > Some of your elog(ERROR)s should probably be ereport(), and I'm not > sure > we want all the elog(DEBUG1)s. > I added them during development. I am not married to them, but found them useful during testing. If we come to the conclusion they're not really that important, i drop them entirely. Thanks for your comments! Bernd