alloc_buffer: Return unqualified pointer type in alloc_buffer_next
Commit Message
* Carlos O'Donell:
> On 3/8/19 3:46 PM, Florian Weimer wrote:
>> alloc_buffer_next is useful for peeking to the remaining part of the
>> buffer and update it, with subsequent allocation (once the length
>> is known) using alloc_buffer_alloc_bytes. This is not as robust
>> as the other interfaces, but it allows using alloc_buffer with
>> string-writing interfaces such as snprintf and ns_name_ntop.
>
> Until now alloc_buffer_next was only used for testing alloc_buffer itself.
>
> Please add a detailed example in the comments for how this API should be
> used. The use case is interesting enough that it needs comments.
>
> OK if you add detailed comments in the header for the use case intended.
Okay. I think this interface should only be used very sparingly, but
I've written a longer comment.
Of course, the patch is now substantially larger than before, so it's a
bit odd that you pre-approved it. 8-/
Thanks,
Florian
alloc_buffer: Return unqualified pointer type in alloc_buffer_next
alloc_buffer_next is useful for peeking to the remaining part of the
buffer and update it, with subsequent allocation (once the length
is known) using alloc_buffer_alloc_bytes. This is not as robust
as the other interfaces, but it allows using alloc_buffer with
string-writing interfaces such as snprintf and ns_name_ntop.
2019-04-08 Florian Weimer <fweimer@redhat.com>
* include/alloc_buffer.h (alloc_buffer_alloc_bytes): Update
comment.
(alloc_buffer_next): Change return type to non-const. Update
comment.
Comments
On 4/8/19 11:29 AM, Florian Weimer wrote:
> * Carlos O'Donell:
>
>> On 3/8/19 3:46 PM, Florian Weimer wrote:
>>> alloc_buffer_next is useful for peeking to the remaining part of the
>>> buffer and update it, with subsequent allocation (once the length
>>> is known) using alloc_buffer_alloc_bytes. This is not as robust
>>> as the other interfaces, but it allows using alloc_buffer with
>>> string-writing interfaces such as snprintf and ns_name_ntop.
>>
>> Until now alloc_buffer_next was only used for testing alloc_buffer itself.
>>
>> Please add a detailed example in the comments for how this API should be
>> used. The use case is interesting enough that it needs comments.
>>
>> OK if you add detailed comments in the header for the use case intended.
>
> Okay. I think this interface should only be used very sparingly, but
> I've written a longer comment.
>
> Of course, the patch is now substantially larger than before, so it's a
> bit odd that you pre-approved it. 8-/
I pre-approved because it's just adding comments and I trust your
judgement there and so am happy to keep the reviews moving forward
in this way. Are you thinking of doing something to loose my trust? ;-)
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> alloc_buffer: Return unqualified pointer type in alloc_buffer_next
>
> alloc_buffer_next is useful for peeking to the remaining part of the
> buffer and update it, with subsequent allocation (once the length
> is known) using alloc_buffer_alloc_bytes. This is not as robust
> as the other interfaces, but it allows using alloc_buffer with
> string-writing interfaces such as snprintf and ns_name_ntop.
>
> 2019-04-08 Florian Weimer <fweimer@redhat.com>
>
> * include/alloc_buffer.h (alloc_buffer_alloc_bytes): Update
> comment.
> (alloc_buffer_next): Change return type to non-const. Update
> comment.
>
> diff --git a/include/alloc_buffer.h b/include/alloc_buffer.h
> index f27cbb65ca..9c469b9e8b 100644
> --- a/include/alloc_buffer.h
> +++ b/include/alloc_buffer.h
> @@ -183,7 +183,7 @@ alloc_buffer_add_byte (struct alloc_buffer *buf, unsigned char b)
> NULL is returned if there is not enough room, and the buffer is
> marked as failed, or if the buffer has already failed.
> (Zero-length allocations from an empty buffer which has not yet
> - failed succeed.) */
> + failed succeed.) The buffer contents is not modified. */
> static inline __attribute__ ((nonnull (1))) void *
> alloc_buffer_alloc_bytes (struct alloc_buffer *buf, size_t length)
> {
> @@ -300,11 +300,32 @@ __alloc_buffer_next (struct alloc_buffer *buf, size_t align)
> /* Like alloc_buffer_alloc, but do not advance the pointer beyond the
> object (so a subseqent call to alloc_buffer_next or
> alloc_buffer_alloc returns the same pointer). Note that the buffer
> - is still aligned according to the requirements of TYPE. The effect
> - of this function is similar to allocating a zero-length array from
> - the buffer. */
> + is still aligned according to the requirements of TYPE, potentially
> + consuming buffer space. The effect of this function is similar to
> + allocating a zero-length array from the buffer.
> +
> + It is possible to use the return pointer to write to the buffer and
> + consume the written bytes using alloc_buffer_alloc_bytes (which
> + does not change the buffer contents), but the calling code needs to
> + perform manual length checks using alloc_buffer_size. For example,
> + to read as many int32_t values that are available in the input file
> + and can fit into the remaining buffer space, you can use this:
> +
> + int32_t array = alloc_buffer_next (buf, int32_t);
> + size_t ret = fread (array, sizeof (int32_t),
> + alloc_buffer_size (buf) / sizeof (int32_t), fp);
> + if (ferror (fp))
> + handle_error ();
> + alloc_buffer_alloc_array (buf, int32_t, ret);
> +
> + The alloc_buffer_alloc_array call makes the actually-used part of
> + the buffer permanent. The remaining part of the buffer (not filled
> + with data from the file) can be used for something else.
> +
> + This manual length checking can easily introduce errors, so this
> + coding style is not recommended. */
Perfect. This is exactly the kind of comment with guidance I was interested
in having documented. Thanks.
> #define alloc_buffer_next(buf, type) \
> - ((const type *) __alloc_buffer_next \
> + ((type *) __alloc_buffer_next \
> (buf, __alloc_buffer_assert_align (__alignof__ (type))))
>
> /* Internal function. Allocate an array. */
>
@@ -183,7 +183,7 @@ alloc_buffer_add_byte (struct alloc_buffer *buf, unsigned char b)
NULL is returned if there is not enough room, and the buffer is
marked as failed, or if the buffer has already failed.
(Zero-length allocations from an empty buffer which has not yet
- failed succeed.) */
+ failed succeed.) The buffer contents is not modified. */
static inline __attribute__ ((nonnull (1))) void *
alloc_buffer_alloc_bytes (struct alloc_buffer *buf, size_t length)
{
@@ -300,11 +300,32 @@ __alloc_buffer_next (struct alloc_buffer *buf, size_t align)
/* Like alloc_buffer_alloc, but do not advance the pointer beyond the
object (so a subseqent call to alloc_buffer_next or
alloc_buffer_alloc returns the same pointer). Note that the buffer
- is still aligned according to the requirements of TYPE. The effect
- of this function is similar to allocating a zero-length array from
- the buffer. */
+ is still aligned according to the requirements of TYPE, potentially
+ consuming buffer space. The effect of this function is similar to
+ allocating a zero-length array from the buffer.
+
+ It is possible to use the return pointer to write to the buffer and
+ consume the written bytes using alloc_buffer_alloc_bytes (which
+ does not change the buffer contents), but the calling code needs to
+ perform manual length checks using alloc_buffer_size. For example,
+ to read as many int32_t values that are available in the input file
+ and can fit into the remaining buffer space, you can use this:
+
+ int32_t array = alloc_buffer_next (buf, int32_t);
+ size_t ret = fread (array, sizeof (int32_t),
+ alloc_buffer_size (buf) / sizeof (int32_t), fp);
+ if (ferror (fp))
+ handle_error ();
+ alloc_buffer_alloc_array (buf, int32_t, ret);
+
+ The alloc_buffer_alloc_array call makes the actually-used part of
+ the buffer permanent. The remaining part of the buffer (not filled
+ with data from the file) can be used for something else.
+
+ This manual length checking can easily introduce errors, so this
+ coding style is not recommended. */
#define alloc_buffer_next(buf, type) \
- ((const type *) __alloc_buffer_next \
+ ((type *) __alloc_buffer_next \
(buf, __alloc_buffer_assert_align (__alignof__ (type))))
/* Internal function. Allocate an array. */