On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kgiu...@redhat.com> wrote:

> Rafi, I agree with the rational behind these changes.
>
> >     /** Return a pointer to a transport's input buffer. This pointer
> >     may
> >      * change when calls into the engine are made.
>
> I think we'll need to be a little more liberal with the lifecycle
> guarantee of these buffer pointers.  Drivers based on "completion" models
> (rather than sockets) could be forced to do data copies rather than
> supplying the pointer directly to the completion mechanism.
>

That sentence was actually supposed to be deleted. The sentences after that
describes the intended lifecycle policy for the input buffer:

  Calls to ::pn_transport_push may change the value of this pointer and the
amount of free space reported by ::pn_transport_capacity.


>
> Could we instead guarantee that pointers (and space) returned from the
> transport remain valid until 1) the transport is released or 2) the
> "push"/"pop" call is made against the transport?
>

That is in fact what I intended for push. For pop this would place a lot
more restrictions on the engine implementation. Say for example peek is
called and then the top half is used to write message data. Ideally there
should actually be more data to write over the network, which means that
the transport may want to grow (realloc) the output buffer, and this of
course is more complex if the external pointer needs to stay valid. Given
that at worst this will incur an extra copy that is equivalent to what
we're currently doing, I figured it would be safer to start out with more
conservative semantics. We can always relax them later when we have had
more time to consider the implementation.


> Or perhaps a reference count-based interface would be safer?  Once the
> driver determines there is capacity/pending, it "reserves" the buffer and
> is required to "push"/"pop" to release it?
>
> Oh, and those names absolutely stink. ;)  It's unclear from the function
> names what components of the transport they are affecting.  I'd rather
> something more readable:
>
> pn_transport_capacity() --> pn_transport_input_capacity()
> pn_transport_buffer() -> pn_transport_input_buffer()
> pn_transport_push() --> pn_transport_input_written()
>

I think your names (and my documentation) actually suffer from exposing too
much of the implementation. There aren't necessarily distinct input/output
buffer objects, rather the whole transport interface itself is really just
single structure (a [transforming] byte queue) with push/peek/pop
corresponding exactly to any standard queue interface.

FWIW, I do agree pn_transport_buffer() could be better, however capacity,
pending, push, peek, and pop all correspond to pretty much every standard
queue/circular buffer/stack interface I've ever seen, and my hasty
documentation notwithstanding, I think that is a good meme to take
advantage of, particularly since the push/peek/pop semantics and their
relationship to each other is actually quite important in guaranteeing that
the pitfall occurring in PROTON-222 doesn't come up when people code their
own drivers.

--Rafael

Reply via email to