[PATCH] ctdb: Introduce buffer.offset to avoid memmove

[Martin Schwenke]

Hi Swen,

On Thu, 26 Jul 2018 13:24:51 +0200, Swen Schillig via samba-technical
<samba-technical at lists.samba.org> wrote:

> Please review and push if happy.
> 
> Thanks for your support in advance.

Another good improvement.

I think I would have missed the first corner case where there isn't
enough space available to read the packet size.  ;-)

The comparisons that check whether a memmove() is necessary are done
differently and this hurts my head.

This makes me wonder whether we can avoid the duplicate logic
altogether and just have something like this for the 2nd instance:

shift:
	/*
	 * If there isn't enough space available at the current offset
	 * to read the packet size or to read the the whole packet
	 * then shift the packet back to the beginning of the buffer
	 */
	avail = queue->buffer.size - queue->buffer.offset;
	if (sizeof(pkt_size) > avail || pkt_size > avail) {
		memmove(queue->buffer.data,
			queue->buffer.data + queue->buffer.offset,
			queue->buffer.length);
		queue->buffer.offset = 0;
	}

Then the 1st instance could just be replaced by "goto shift" instead of
"goto data_read".  The only gotcha is that pkt_size then needs to be
initialised to 0 to avoid a (slightly clueless but understandable)
compiler warning.

What do you think?

If you hate that idea then I can live with the duplication, but can the
1st condition to please be changed to:

  sizeof(pkt_size) > queue->buffer.size - queue->buffer.offset

to make it consistent with the 2nd similar check?

Thanks...

peace & happiness,
martin