alloc_buffer: Return unqualified pointer type in alloc_buffer_next

Message ID 20190308204633.49DCE80DD6B5@oldenburg2.str.redhat.com
State Superseded
Headers

Commit Message

Florian Weimer March 8, 2019, 8:46 p.m. UTC
  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-03-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.
  

Comments

Florian Weimer April 8, 2019, 10:23 a.m. UTC | #1
* Florian Weimer:

> 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-03-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.

Ping?  <https://sourceware.org/ml/libc-alpha/2019-03/msg00179.html>

Thanks,
Florian
  
Carlos O'Donell April 8, 2019, 2:34 p.m. UTC | #2
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.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-03-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.
> 
> diff --git a/include/alloc_buffer.h b/include/alloc_buffer.h
> index f27cbb65ca..7f68f46eac 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.  */

s/is/are/g

>   static inline __attribute__ ((nonnull (1))) void *
>   alloc_buffer_alloc_bytes (struct alloc_buffer *buf, size_t length)
>   {
> @@ -302,9 +302,13 @@ __alloc_buffer_next (struct alloc_buffer *buf, size_t align)
>      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.  */
> +   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.  */

This needs a detailed example in the header comments like the other API usage
examples. This use is odd enough that it needs a good comment.

>   #define alloc_buffer_next(buf, type)				\
> -  ((const type *) __alloc_buffer_next				\
> +  ((type *) __alloc_buffer_next					\

OK.

>      (buf, __alloc_buffer_assert_align (__alignof__ (type))))
>   
>   /* Internal function.  Allocate an array.  */
>
  

Patch

diff --git a/include/alloc_buffer.h b/include/alloc_buffer.h
index f27cbb65ca..7f68f46eac 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)
 {
@@ -302,9 +302,13 @@  __alloc_buffer_next (struct alloc_buffer *buf, size_t align)
    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.  */
+   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.  */
 #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.  */