On 09/04/2025 13:46, Heikki Linnakangas wrote:
On 09/04/2025 13:28, Heikki Linnakangas wrote:
On 09/04/2025 12:39, Peter Eisentraut wrote:
On 09.04.25 10:53, Heikki Linnakangas wrote:
On 08/04/2025 22:41, Heikki Linnakangas wrote:
On 08/04/2025 20:06, Peter Eisentraut wrote:
While I was looking at this, I suggest to make the first argument
void *. This is consistent for passing binary data.
Ok, sure.
On second thoughts, -1 on that. 'void *' is appropriate for
functions like libc's read() or pq_sendbytes(), where the buffer can
point to anything. In other words, the caller is expected to have a
pointer like 'foobar *', and it gets cast to 'void *' when you call
the function. That's not the case with the cancellation key. The
cancellation key is just an array of bytes, the caller is expected
to pass an array of bytes, not a struct.
The right precedent for that are e.g. SCRAM functions in scram-
common.h, for example. They use "const uint8 *" for the hashes.
I'll switch to "const uint *" everywhere that deals with cancel
keys. There are a few more variables elsewhere in the backend and in
libpq.
I was having the same second thoughts overnight. I agree with your
conclusion.
Here's a patch to change cancellation keys to "uint8 *". I did the
same for a few other places, namely the new scram_client_key_binary
and scram_server_key_binary fields in pg_conn, and a few libpq
functions that started to give compiler warnings after that. There
probably would be more code that could be changed to follow this
convention, but I didn't look hard. What do you think?
I'm on the edge with the pg_b64_encode/decode functions, whether they
should work on "uint8 *" or "void *". On one hand, you do base64
encoding on a byte array, which would support "uint8 *". But on the
other hand, you might use it for encoding things with more structure,
which would support "void *". I went with "void *", mostly out of
convenience as many of the SCRAM functions that currently use
pg_b64_encode/decode, use "char *" to represent byte arrays. But
arguably those should be changed to use "uint8 *" too.
I went around looking a bit more anyway. Here's a patch to change more
places to use 'uint8' for byte arrays, in SCRAM and MD5 salts and
digests and such. It's a bit of code churn, but I think it improves
readability. Especially the SCRAM code sometimes deals with base64-
encoded string representations of digests and sometimes with decoded
byte arrays, and it's helpful to use different datatypes for them.
Polished this up a tiny bit, and committed.
--
Heikki Linnakangas
Neon (https://neon.tech)
