Re: pgcrypto: PGP armor headers

From: Marko Tiikkaja <marko(at)joh(dot)to>
To: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pgcrypto: PGP armor headers
Date: 2014-09-10 11:26:58
Message-ID: 54103582.5030203@joh.to
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 9/9/14 11:01 AM, I wrote:
> On 9/9/14 10:54 AM, Heikki Linnakangas wrote:
>> So I think this (and the corresponding dearmor code too) should be
>> refactored to use a StringInfo that gets enlarged as needed, instead of
>> hoping to guess the size correctly beforehand. To ease review, might
>> make sense to do that as a separate patch over current sources, and the
>> main patch on top of that.
>
> Yeah, I thought the same thing while writing that code. Thanks, I'll do
> it that way.

Attached is what I have right now. I started working on the decoding
part, but it has this piece of code:

/* decode crc */
if (b64_decode(p + 1, 4, buf) != 3)
goto out;

which makes the approach a bit uglier. If I did this the same way, I
would have to create and destroy a StringInfo just for this operation,
which seems ugly. So I wonder if I shouldn't try and instead keep the
code closer to what it is in HEAD right now; I could call
enlargeStringInfo() first, then hand out a pointer to b64_encode (or
b64_decode()) and finally increment StringInfoData.len by how much was
actually written. That would keep the code changes a lot smaller, too.

Is either of these approaches anywhere near what you had in mind?

I'm also not sure why we need to keep a copy of the base64
encoding/decoding logic instead of exporting it in utils/adt/encode.c.

.marko

Attachment Content-Type Size
pgcrypto_armor_stringinfo.patch text/plain 6.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Heikki Linnakangas 2014-09-10 11:38:04 Re: pgcrypto: PGP armor headers
Previous Message Fujii Masao 2014-09-10 11:13:50 Re: Add shutdown_at_recovery_target option to recovery.conf