runit: buffer_feed()

runit: buffer_feed()

[Tomas Carnecky]

I'm looking at the buffer_feed() function in runit and I can't make 
sense of it. I'm in the process of replacing all byte_* functions with 
standard memcpy/memcmp etc, but I just don't see what this byte_copyr() 
is supposed to do:

int buffer_feed(buffer *s)
{
   int r;

   if (s->p) return s->p;
   r = oneread(s->op,s->fd,s->x,s->n);
   if (r <= 0) return r;
   s->p = r;
   s->n -= r;
   if (s->n > 0) byte_copyr(s->x + s->n,r,s->x);
   return r;
}

byte_copyr() copies backwards, so you're reading/accessing bytes 
_before_ s->x. Shouldn't that cause a SIGSEGV?
Also, what is ->p and ->n? ->n seems to be the offset into the ->s char 
array, ->p the number of bytes currently in the buffer. It looks that 
way if I look at buffer_peek(), but ->n is initialized to 'len', which 
means that buffer_peek() just after the buffer is initialized returns a 
pointer beyond the end of the buffer.. ? Also, ->n is decreased after 
data is read into the buffer (using oneread). That doesn't make sense. 
It's all very confusing.

Can sombody please explain how the code is supposed to work?

thanks
tom

Re: runit: buffer_feed()

[Charlie Brady]

On Fri, 23 Jan 2009, Tomas Carnecky wrote:

> I'm looking at the buffer_feed() function in runit and I can't make sense of 
> it. I'm in the process of replacing all byte_* functions with standard 
> memcpy/memcmp etc,

Why?

Re: runit: buffer_feed()

[Tomas Carnecky]

On 01/23/2009 03:30 PM, Charlie Brady wrote:
>
> On Fri, 23 Jan 2009, Tomas Carnecky wrote:
>
>> I'm looking at the buffer_feed() function in runit and I can't make
>> sense of it. I'm in the process of replacing all byte_* functions with
>> standard memcpy/memcmp etc,
>
> Why?

byte_copy is a poor reimplementation of memcpy, and memcpy is part of 
any libc runtime (according to the man page, conforming to: SVr4, 
4.3BSD, C89, C99, POSIX.1-2001.). Please point my to a single libc that 
implements malloc, open, write, fork but not memcpy.
Also, it makes the executable smaller and faster, the project as a whole 
faster to compile, the code easier to read etc.

But nvm, I found out what I was missing. The code still doesn't make 
sense to me, but at least the resulting executables works correctly.

tom

Re: runit: buffer_feed()

[Laurent Bercot]

> byte_copy is a poor reimplementation of memcpy

 Wrong.
 byte_copy works when the source and destination overlap and dst < src.
 byte_copyr works when the source and destination overlap and dst > src.
 memcpy is NOT guaranteed to work when the source and destination overlap.

 If you want an optimized function that works in every case, try memmove.


> Also, it makes the executable smaller and faster, the project as a whole 
> faster to compile, the code easier to read etc.

 Your mileage may vary. I'm used to the byte_copy() interface. :)

 I'm not pretending your remarks are unfounded. The functions in byte.h
and buffer.h are very old DJB code, when he was rewriting the world and
its mother in order to be portable on alien computers pretending to be
half POSIX-compliant.
 I'm not aware of any modern architecture/libc combination where memmove()
doesn't work, so I *think* byte_copy and byte_copyr's naive implementations
can be safely replaced with memmove() (and that's what skalibs does by
default). I could be wrong, though, and be non-portable on a few archs.
 As for buffer.h, I disliked DJB's implementation and rewrote it entirely.
The API, however, is excellent.
 (If I'm not mistaken, buffer_feed() fills the buffer if it's empty (and
doesn't if it's not), and returns the number of bytes in the buffer.)

 What I'm saying is there's risk involved in patching software without
giving it a good deal of thought - especially software including DJB code,
which usually features a number of non-apparent, but important, design
decisions. If you go and wildly replace byte_copy with memcpy, you will
introduce bugs into the code. ;)

-- 
 Laurent