svn commit: samba r7025 - in branches/SAMBA_3_0/source: lib tdb

[derrell at samba.org]

Jeremy Allison <jra at samba.org> writes:

> On Fri, May 27, 2005 at 12:54:12PM -0400, derrell at samba.org wrote:
>> This change to lib/util_sock.c that I checked in (r7025) should be sanity
>> checked.  I asked a few people to sanity check it a couple of weeks ago, but I
>> got feedback only from tridge.  I've been using this for a couple of weeks
>> with no problem, but since it is at the very core of samba functionality,
>> others should concur that it doesn't break anything.
>
> It doesn't look right. You're using read_socket_data(), which has
> some very definate properties.
>
> Firstly, it has only 3 possible returns :
>
> 1). 0. In this case smb_read_error gets set to EOF.
>
> 2). -1. In this case smb_read_error gets set to READ_ERROR.
> Also, as read_socket_data() calls sys_read() within a loop
> then it's possible we had a partial read which could then
> be lost. This is a bug in read_socket_data() I think.
>
> 3). N. N is *guarenteed* to be the number of bytes asked for,
> due to read_socket_data() calling sys_read() in a loop.
>
> Note that in no case can it return a partial read, making all
> the extra code you added meaningless.

You're right that it can not return a partial read (that's a different bug, I
think) but it CAN consume a portion of a packet (having iterated through the
loop 1 or more times already) and then have an interrupt cause it to return
-1.  Therefore, I do not believe that the extra code is meaningless.  Something
of the sort needs to go either where I put it, or in read_socket_data().

The problem is that read_socket_data() is SUPPOSED to return if an interrupt
occurs (I think), but receive_smb_raw() is SUPPOSED obtain the whole packet,
retrying, if necessary, any interrupted reads.

> The real problem is within read_socket_data() I think. Please
> revert this change and let's rethink this.

Rather than reverting the patch, since I think it's mostly correct, let's
figure out what might be wrong with it and I'll fix that.  It's better off
right now than how it was, I think, so it's no harm leaving it.

It seems to me that read_socket_data() should not return -1 if any data has
been successfully read and sys_read() returns -1 and errno is EAGAIN or EINTR.
Instead, it should return 'total', the amount of data read so far.  This
changes the semantics of read_socket_data(), though (although
read_socket_data() in combination with receive_smb_raw() is already broken).

The alternative that I think maybe you're proposing is for read_socket_data()
to look at errno and if EAGAIN and EINTR, continue looping (which would make
the additional code in receive_smb_raw() unnecessary.

Which of these (or some other alternative) do you prefer?  I'll incorporate
whatever we decide on today.

Derrell