Date: Fri, 9 Oct 2020 11:48:59 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: iwata.aya@fujitsu.com, pgsql-hackers@postgresql.org, tgl@sss.pgh.pa.us,
	robertmhaas@gmail.com, pchampion@pivotal.io, jdoty@pivotal.io,
	raam.soft@gmail.com, nagaura.ryohei@fujitsu.com,
	nagata@sraoss.co.jp, peter.eisentraut@2ndquadrant.com,
	horiguchi.kyotaro@lab.ntt.co.jp, k.jamison@fujitsu.com
Subject: Re: libpq debug log
Message-ID: <20201009144859.GA31144@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201009.173122.1700952870250126070.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Oct-09, Kyotaro Horiguchi wrote:

> I saw that version and have some comments.
> 
> +pqGetProtocolMsgType(unsigned char c, PGCommSource commsource)
> +{
> +	const char *message_type;
> 
> Compiler complains that this is unused.
> 
> +static const char *
> +pqGetProtocolMsgType(unsigned char c, PGCommSource commsource)
> +{
> ...
> +	else
> +		return "UnknownCommand";
> +}
> 
> Compiler complains as "control reached end of non-void function"

Yeah, those two warnings are caused by the same problem, namely that I
was editing this function to make it simpler and apparently the patch
version I sent does not include all such changes.  The fix is to remove
the message_type variable and have the two assignments be "return".

> +pqLogMsgString(PGconn *conn, const char *v, int length, PGCommSource commsource)
> +{
> +	if (length < 0)
> +		length = strlen(v) + 1;
> +

> pqLogMsgString(conn, str, -1, FROM_*) means actual length may be
> different from the caller thinks, but the pqLogLineBreak() subtracts
> that value from the message length rememberd in in logging_message.
> Anyway AFAICS the patch doesn't use the code path so we should remove
> the first two lines.

True, +1 for removing it.

> By the way, appendBinaryPQExpBuffer() enlarges its buffer by the size
> of the exact length of the given data, but appends '\0' at the end of
> the copied data. Couldn't that leads to an memory overrun?

Doesn't enlargePQExpBuffer() include room for the trailing zero?  I
think it does.