Re: [Libevent-users] Send evbuffer to multiple clients

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Tue, Jun 7, 2011 at 7:08 PM, Joachim Bauch <jojo@xxxxxxxxxxx> wrote:
> Am Montag, den 06.06.2011, 20:29 -0400 schrieb Nick Mathewson:
>> Hm.  If there's nothing hideously complex in the source buffer (that
>> is, nothing added with evbuffer_add_file), you could try an
>> appropriate combination of evbuffer_peek and evbuffer_add_reference.
>> Other than that, I'm not thinking of much.
>>
>> I'd love to get a better API for this in 2.1, if anybody wants to write one.
>
> Attached is a patch against the current git head that adds a new method
> "evbuffer_add_buffer_reference" providing a non-destructive add from one
> evbuffer to another while avoiding unnecessary copies. For now it only
> supports buffers containing only memory chains (i.e. no file segments
> and sendfile chains). Also included is a first testcase, the default
> regression tests still pass.

Neat!

So if I understand here, the semantics are that once a chain has been
added by reference to another evbuffer, the chain is now supposed to
be immutable.  Could we simplify stuff by also declaring that if you
have an evbuffer which is added by reference to another evbuffer, the
entire original evbuffer is immutable?  It seems to me that would make
the rules for what you can use these for much easier to understand,
and it would prevent people from doing crazy stuff.

Is there a reason that EVBUFFER_MEM_PINNED_M needs to be distinct from
EVBUFFER_IMMUTABLE?

I think there's a race condition around evbuffer_chain_decref.  The
reference count needs to be protected by the lock of the evbuffer that
contains the referenced-chain, but right now it looks like it's only
protected by the lock of the evbuffer that contains the referencing
chain.

I think we can move the "parent" field into a type-specific
EVBUFFER_CHAIN_EXTRA() annex, in the way that evbuffer_chain_reference
and evbuffer_chain_file_segment and evbuffer_file_segment currently
do.

It would be fine to do this one in another patch, but
evbuffer_file_segment should actually be copyable with the appropriate
bookkeeping.

Also, I had considered another possible implementation.  Instead of
adding a reference to each chain in the referenced buffer, we could
instead add a reference to the referenced buffer itself, and use an
evbuffer_ptr to track how far into it we were.  But on consideration,
I think your approach is better: my idea would make too many functions
harder to implement, without much real gain.

> As this is my first attempt to change something in the buffer internals,
> could you please check for any obvious errors or wrong coding style?
> I hope sending this to the list is okay, or should I post it somewhere
> else?

The list is a great place to discuss patches, but IMO it has the
problem that stuff can get forgotten.  Sourceforge and github are
better at preventing stuff from getting forgotten, but fewer people
read them, alas.  Let's keep talking here for now, but if one of us
drops the ball on replying, let's remember to add a ticket/pull
request/whatever for this.

Also fwiw, the best format for sending patches is the one generated by
"git format-patch"; it makes them much easier to merge.  Pointing at a
branch on a public git repository is also pretty good.

many thanks,
-- 
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.