tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kevin seguin <seg...@motive.com>
Subject Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/util/buf ByteChunk.java
Date Sun, 03 Jun 2001 20:05:28 GMT
"Ignacio J. Ortega" wrote:
> 
> > perhaps i'm missing something, but it looks like the offset
> > returned by
> > getOffset() should be the offset into the byte array contained in
> > ByteChunk, which would be the start position in this case, rather than
> > the end position.
> >
> 
> From the point of view of the o.a.t.m.s.ajp13, now getOffset() has the
> correct behavior, at least now for me works, prior to that change ajp13
> was unusable.., take a look in o.a.t.m.s.Ajp13.Ajp13Packet.appendString,
> There is some magic in the use of ob.setbyteOff and ob.getByteOff...as i
> understand this code.. ob.getByteOff is requiered to return the end of
> the last writed byte chunk ..perhaps a better aproach to this fix is to
> change ob.getOffset to use  bb.getEnd instead os bb.getOffset..at least
> this will make the intention clearer.., in a really obscure code..
> 

i see...  kind of makes my head hurt :)  i think you're probably right
about changing OutputBuffer.getOffset() -- i would make things clearer.

> > also, if you look at the append() and equals() methods that take
> > ByteChunk's as arguments, if getOffset() returns the end
> > position rather
> > than the start position, these methods won't work as intended.  for
> > example, equals() will start comparing comparing bytes at the
> > end of the
> > ByteChunk, rather than the start of it.
> >
> 
> I reviewed it, and your are right for equals..but not for append..append
> is suppoused to use the end of the buffer not the start..so in append
> getOffset is suppoused to return end not the start...
> 

are you sure?  here's the code i was looking at:

---- code snip ----
    public void append( ByteChunk src )
	throws IOException
    {
	append( src.getBytes(), src.getOffset(), src.getLength());
    }

    /** Add data to the buffer
     */
    public void append( byte src[], int off, int len )
	throws IOException
    {
	// will grow, up to limit
	makeSpace( len );

	// if we don't have limit: makeSpace can grow as it wants
	if( limit < 0 ) {
	    // assert: makeSpace made enough space
	    System.arraycopy( src, off, buff, end, len );
	    end+=len;
	    return;
	}

	// if we have limit and we're below
	if( len <= limit - end ) {
	    // makeSpace will grow the buffer to the limit,
	    // so we have space
	    System.arraycopy( src, off, buff, end, len );
	    end+=len;
	    return;
	}
---- end code snip ----

it looks like when you call append(ByteChunk), the result will be an
IndexOutOfBoundsException.  in fact, this code proves that:

    public void testByteChunk() throws Exception {
        System.out.println("testByteChunk");

        byte[] b1 = new String("bytes1").getBytes();
        byte[] b2 = new String("bytes2").getBytes();

        ByteChunk bc1 = new ByteChunk();
        ByteChunk bc2 = new ByteChunk();

        bc1.setBytes(b1, 0, b1.length);
        bc2.setBytes(b2, 0, b2.length);

        String s1 = bc1.toString();
        String s2 = bc2.toString();

        System.out.println("s1 = " + s1);
        System.out.println("s2 = " + s2);

        bc1.append(bc2);
        s1 = bc1.toString();
        System.out.println("s1 = " + s1);
    }

i believe the solution here would be to have append(ByteChunk) look like
this:

    public void append( ByteChunk src )
	throws IOException
    {
	append( src.getBytes(), src.getStart(), src.getLength());
    }


> The better solution is to get rid of getOffset..and only use getStart or
> getEnd where needed...
>
> > just a sanity check...
> >
> 
> Well done, OSS is great because of this comments..and the possibility to
> make them just-in-time..:)))
> 
> > -kevin.
> 
> Saludos ,
> Ignacio J. Ortega

Mime
View raw message