Re: Request for Feedback: new BIO API

[Christian Grothoff]

On 5/11/20 4:10 PM, Alessio Vanni wrote:
> (For some reason, I'm not receiving all the mails from this list.  Some
> are arriving, some not.  Not sure what is going on, if there's an issue
> in a configuration somewhere or simply a network problem; still, I had
> to reply like this because I didn't receive anything and noticed the
> reply only because I checked the web archive.  This mail is in reply to
> Christian.)

Likely firemail.cc runs some way too aggressive spam filter? Anyway,
cc'ing you directly may help...

>> 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.
> 
> The `write' side of the API has an upper bound because it was initially
> designed to be used on already-allocated buffers, so something like
> this:
> 
> char *buffer = GNUNET_new_array (some_size, char);
> /* Maybe something going on here */
> struct GNUNET_BIO_WriteHandle *wh = GNUNET_BIO_write_create (buffer, 
> some_size);
> /* Write operations here */
> 
> but admittedly using an internally-allocated buffer which is returned on
> destroy is a better design. However, I think the `GNUNET_Buffer' API is
> a bit underfeatured: to begin with, the function "reaping" the data
> returns a 0-terminated string, which in this case is the wrong
> representation.  It's probably trivial to implement a function returning
> a `void *', though (I don't actually know how GNUNET_Buffers are
> implemented as I have yet to read that part of the codebase.)

Indeed, simply adding a GNUNET_buffer_reap() that returns a void * +
size_t instead of a C-string would do the trick, and should be trivial.

>> The 'h' can't be in there, it should be provided only when the 'spec'
>> is executed
> 
> The idea was to have something like this:
> 
> struct GNUNET_BIO_ReadHandle *h1 = GNUNET_BIO_read_open ("somefile");
> struct GNUNET_BIO_ReadHandle *h2 = GNUNET_BIO_read_create (some_buffer, 
> some_size);
> struct GNUNET_BIO_ReadSpec r[] = {
>  GNUNET_BIO_read_spec_string (h1, "string11", &some_string1, max_len1),
>  GNUNET_BIO_read_spec_string (h1, "string12", &some_string2, max_len2),
>  GNUNET_BIO_read_spec_string (h2, "string21", &some_string3, max_len3),
>  GNUNET_BIO_read_spec_string (h2, "string22", &some_string4, max_len4),
>  GNUNET_BIO_read_spec_end (),
> };
> GNUNET_BIO_read_spec_commit (r);

Right, but that means I have to repeat the hx lots, and can't re-use the
same spec as easily. I would have used:

struct GNUNET_BIO_ReadHandle *h1
   = GNUNET_BIO_read_open ("somefile");
struct GNUNET_BIO_ReadSpec spec[] = {
  GNUNET_BIO_read_spec_string ("string11", &some_string1, max_len1),
  GNUNET_BIO_read_spec_string ("string12", &some_string2, max_len2),
  GNUNET_BIO_read_spec_end (),
};

GNUNET_BIO_read_spec_commit (h1, spec);

> instead of having to declare multiple arrays for multiple operations.

I think it is rare to have multiple input streams to be processed at the
same time. The above also offers a simpler way to change 'h1' to
something else as it is only needed in one place.

> Also I'm not sure what you mean by "and/or depend on the 'type' (thus
> should be part of the internal knowledge of the callback!)".  `h' is the
> Read/WriteHandle, so if it's part of the callback it would be the same
> as what I implemented. Though I might've simply misunderstood.

I'm trying to say that different callback functions would be called for
the different types (so no 'switch' but instead effectively 'virtual
dispatching').

> As for the callback-based structure... that's probably a better design,
> yes.  I'll work on a second version of the patch and submit it again
> when it's ready.

Great!

Happy hacking!

Christian

signature.asc
Description: OpenPGP digital signature