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

* 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

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.  */
>

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

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.  */
 #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.  */

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.  */
>