Re: [PATCH] libpipe/pq.c

[Marcus Brinkmann]

On Wed, Apr 10, 2002 at 01:48:08PM +0300, Ognyan Kulev wrote:
> 
> I don't know how this library is used so it is possible that a part of 
> its interfaces demand that ports are already removed by 
> `packet_read_ports'.  But why then `pq_dequeue' calls 
> `packet_dealloc_ports'?  So i think the correct is to set num_ports to zero.

Yes.  S_socket_recv in pflocal does read the ports via pipe_recv, but it
uses them in the return message with MACH_MSG_TYPE_MAKE_SEND, so new send
rights are created.  The existing reference is not consumed, so deallocating
them in pq_dequeue is the right thing to do.

> There are many places where `bcopy' is used instead of `memcpy'.

As said before, we clean them up as we go.  Feel free to GC those whenever
you work on some code (but it's not important to send a patch just for
that).

> The second question is about using mmap/munmap instead of 
> vm_allocate/vm_deallocate.  In pq.[ch] they are mixed, there is no 
> consistency.  What is the reason to prefer `mmap' to `vm_allocate'?  Is 
> there a policy about that?

Also as said before, we use munmap consistently when possible, to become
less dependent on Mach.

I am not sure how to test this code.  You need to send port rights over
sockets, so for example a program exercising file descriptor passing would
use this code.  Also, the lack of initialization of buf_start and buf_end
should show up whenever packets are reused, but I am not sure when this
happens (IOW: why didn't we see a problem here earlier).

I think that initialization of num_ports and buf_start/buf_end can be better
done outside the if/else branches, as it is the same for both.  But that isd
only a small clean up.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de