Re: Request for Feedback: new BIO API

[Christian Grothoff]

Hi Alessio,

The GNUNET_BIO_write_create() API right now takes a buffer and size,
which presumes that you know apriori how big the buffer will become (or
at least have an upper bound). I think 'write_create()' should just take
*no* arguments, use the GNUNET_Buffer-API internally to allocate as much
space as needed, and then some new 'write_destroy_to_buffer()' function
would *return* the GNUNET-buffer (or a void * and size_t) of the size
that was required for all of the write operations.

(Naturally, write_destroy_to_buffer() could ONLY be called if
write_create()/write_create_for_buffer() was used to setup the
WriteHandle, not if the write was going to a file.)

GNUNET_BIO_SPEC: *If* we do that, we should really use an 'enum'. (and
use that enum also instead of 'int' below). However, instead I would
propose that the 'ReadSpec' should include a callback/function pointer
instead of the type. That will be properly extensible, while an 'enum'
is not. (See the GNUNET_PQ/SQ/MY-APIs, all of those 'spec's have
function pointers.)  I also actually don't quite understand what the
various fields in the read spec are supposed to be about. The 'h' can't
be in there, it should be provided only when the 'spec' is executed
and/or depend on the 'type' (thus should be part of the internal
knowledge of the callback!). So the "ReadSpec" is very different from
what I had imagined.

Consider something more like this:

/**
 * Function to deserialize some data structure, reading from @a rh
 * and storing the result to @a target.
 *
 * @param cls closure
 * @param[in,out] rh handle to a reader to be used to get input bytes
 * @param[in,out] target handler-specific pointer for where to write
 * @param target_size number of bytes in @a target, 0 for
 *                    unknown/variable
 * @return #GNUNET_OK on success, #GNUNET_SYSERR if the read
 *         buffer is malformed. #GNUNET_NO if the read buffer
 *         did not have enough bytes remaining (i.e. due to
 *         data being truncated).
 */
typedef int
(*GNUNET_BIO_ReadHandler)(void *cls,
                          struct GNUNET_BIO_ReadHandle *rh,
                          void *target,
                          size_t target_size);

struct GNUNET_BIO_ReadSpec {
  /**
   * Function that performs the deserialization. (// your type!)
   */
  GNUNET_BIO_ReadHandler rh;

  /**
   * Closure for @e rh, usually NULL.
   */
  void *rh_cls;

  /**
   * For error messages (can be NULL).
   */
  const char *what;

  /**
   * Where to write the data that was read to. May be a pointer
   * to a pointer, especially in case that some data structure
   * must be allocated dynamically.
   */
  void *target;

  /**
   * Size of @e target, 0 for unknown or variable-length,
   * or a limit on the size of target for bounded-length
   * allocations.
   */
  size_t target_size;
};

struct GNUNET_BIO_ReadSpec
GNUNET_BIO_read_spec_fixed_size_object (const char *what,
                             void *result,
                             size_t size);

// *dst will be _set_ by the read function
struct GNUNET_BIO_ReadSpec
GNUNET_BIO_read_spec_string (const char *what,
                             char **dst,
                             size_t max_length);

struct GNUNET_BIO_ReadSpec
GNUNET_BIO_read_spec_uint32 (const char *what,
                             uint32_t *dst);

(note that in all of the above the 'cls' argument would always be NULL).


Same change would apply to the WriteSpec.

Given this, it would be possible to extend BIO _outside_ of util, like
we do for GNUNET_GETOPT, PQ, SQ, and MY APIs already...

Let me know if I'm (still) unclear!

Happy hacking!

Christian


On 5/9/20 2:38 PM, Alessio Vanni wrote:
> Hello everyone,
> 
> as discussed in a different mail exchange[1], the BIO API isn't
> considered a great API and it was suggested to improve on it.  The
> attached patch is an attempt to both improve the API and add
> memory-backed buffers as proposed in the above-mentioned discussion.
> 
> Please note: this patch is not complete; it doesn't have tests (though
> they were made to check that the API works) and most importantly doesn't
> have modifications to port the rest of the code to the new API.  This is
> because this patch is only for review, so I will take any feedback, make
> changes if needed and when we agree on a certain version I will also
> change all the callers to BIO and provide the final patch.
> 
> The patch adds I/O on already-allocated buffers and provides a new API
> similar to the one used for databases, as suggested by Christian.  It
> doesn't add I/O for arrays as I wasn't sure how to implement it, though
> I had made an attempt before yanking it out and some hints at it can be
> seen here and there (though they don't affect the rest of the code.)
> 
> To clarify here one of the design choices, the "spec" API uses the
> handle in the spec definition instead of in the "commit" function so as
> to declare all the operations on multiple sources/destinations all
> together and execute them all in one go.  I think it's fairly convenient
> to do it this way instead of creating multiple specs, but this is just
> my opinion and is not backed by any objective evaluation :)
> 
> Thanks,
> A.V.
> 
> ---
> 
> [1] https://lists.gnu.org/archive/html/gnunet-developers/2020-04/msg00015.html
>

signature.asc
Description: OpenPGP digital signature