[PATCH 1/6] msgb: Add msgb_resize_area and msgb_copy

[Jacob Erlbeck]

On 25.11.2015 15:58, Neels Hofmeyr wrote:

> 
> Actually, msgb's len fields are typed as uint16_t, so I think old_size and
> new_size should be uint16_t arguments...?

I am not fond of using uint16_t in an API for such purposes, the
compiler does not catch overflow, it might even be slower than an int
when passed as an argument. In my opinion this is just a space
optimization for the struct, and should not leak out to the API.
Unfortunately the API is already inconsistent by using a mixture of int,
unsigned int, and uint16_t types for length parameters.

I agree, that size_t should not be added to that list. So the question
remains, whether to use "int" or "unsigned int". K&R evangelists would
probably go for "int", which has the advantage of to be able to compute
differences without changing signedness on the way and to move the
wrapping point far away. "unsigned int" had the advantage of not having
to check for negative values and that it is more explicit in the API
(BTW, msgb_trim uses int and does not check, thanks for the mug ;-)

Prefering the "int" variant I have changed the function to

int msgb_resize_area(struct msgb *msg, uint8_t *area,
			    int old_size, int new_size)
{
	int rc;
	uint8_t *rest = area + old_size;
	int rest_len = msg->len - old_size - (area - msg->data);
	int delta_size = new_size - old_size;

	if (old_size < 0 || new_size < 0)
		MSGB_ABORT(msg, "Negative sizes are not supported\n");
	if (area < msg->data || rest > msg->tail)
		MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n");

        ...
}

which I believe to be much clearer.

> 
> And then, I think these are also necessary:
> 
> [...]
> 
> - assert new_size <= (0xffff - (area - msg->data))
>   (so that the resulting len is within uint16_t).
> 

Note that msgb_trim is responsible for checking the length (which should
be fixed for negative values) and thus the upper limit of new_len.

I was wondering about whether it makes sense, to handle the "buffer too
small" case differently (by returning -1 instead of aborting, like
msgb_trim), but could make sense in cases, where one can react to this
by allocating a new buffer instead of patching the old one instead.

Jacob

-- 
- Jacob Erlbeck <jerlbeck at sysmocom.de>       http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte