This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] alloc_buffer: Return unqualified pointer type in alloc_buffer_next


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



--
Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]