first_write_block() in MIOBuffer

[Brian Geffon <br...@apache.org>]

Hello All,
I think we have a major flaw in first_write_block() in MIOBuffer, the
comment and code are the following:

  /**
    Returns a pointer to the first writable block on the block chain.
    Returns NULL if there are not currently any writable blocks on the
    block list.

  */
  IOBufferBlock *first_write_block()
  {
    if (_writer) {
      if (_writer->next && !_writer->write_avail())
        return _writer->next;
      ink_assert(!_writer->next || !_writer->next->read_avail());
      return _writer;
    } else
        return NULL;
  }


According to this comment it should find the first block with available
space or return NULL, obviously this code only examines two blocks and
(writer and writer->next). I think this code should be more like:

  IOBufferBlock *first_write_block()
  {
    for(IOBufferBlock *cur = _writer; cur != NULL; cur=cur->next) {
      if (cur->write_avail())
        return cur;
    }
    return NULL;
  }

Does anyone have comments on this? I'm not sure if this change will break
anything, so feedback will be appreciated.

Brian

Re: first_write_block() in MIOBuffer

[James Peach <jp...@apache.org>]

On Apr 30, 2013, at 1:38 AM, Nick Kew <ni...@apache.org> wrote:

> 
> On 30 Apr 2013, at 01:44, Brian Geffon wrote:
> 
>> Hello All,
>> I think we have a major flaw in first_write_block() in MIOBuffer, the
>> comment and code are the following:
> 
> 
> Ref: thread on this list about a week ago,
> "TS API won't return header data of > 4K".
> Your patch looks right, but then so did mine.
> 
> If that thread was your startingpoint and you've addressed the
> issues raised there, ignore the rest of this. 
> 
> It not ....
> 
> There are several instances of similar logic there.
> I recently 'fixed' one in MIOBuffer::fill using essentially the same
> logic as yours, only to find side-effects.  Bottom line seems to be,
> yes you can and indeed must move _writer and lose its previous
> block in certain operations.  You'd better have a reader already
> allocated or you'll never again see that block!

A quick followup from the hack day ... the MIOBuffer is designed to be append-only, which is why it assumes that you only need to worry about the current block and the (possible) next block. This means that when you fill(), you have to fill() each block as you append it, so consider just appending a block of the size that you need.

I think that we discussed Nick's point above and agreed that a reader should always be attached before any write happens. IMHO we should enforce this with API or debug assertions.

J

Re: first_write_block() in MIOBuffer

[Nick Kew <ni...@apache.org>]

On 30 Apr 2013, at 01:44, Brian Geffon wrote:

> Hello All,
> I think we have a major flaw in first_write_block() in MIOBuffer, the
> comment and code are the following:


Ref: thread on this list about a week ago,
"TS API won't return header data of > 4K".
Your patch looks right, but then so did mine.

If that thread was your startingpoint and you've addressed the
issues raised there, ignore the rest of this. 

It not ....

There are several instances of similar logic there.
I recently 'fixed' one in MIOBuffer::fill using essentially the same
logic as yours, only to find side-effects.  Bottom line seems to be,
yes you can and indeed must move _writer and lose its previous
block in certain operations.  You'd better have a reader already
allocated or you'll never again see that block!

-- 
Nick Kew