Re: [PATCH] AcceptEx Impelementation

[Kai Blin]

Hi Scott,

I didn't see a reply to your AcceptEx patches yet, sorry if I missed it and 
you get this again. Please submit one patch per email only. 

I admittedly don't know much about the wineserver stuff, but I've noticed a 
couple of style issues in the winsock code:

The indentation common in socket.c is either a two-space indent or a 
four-space indent. Please don't add new code with an eight-space indent.

Also, the common convention for ifs is
if (cond)
{
and not
if(cond){

Apart from those minor nitpicks, the code looks good.

Cheers,
Kai

-- 
Kai Blin
WorldForge developer  http://www.worldforge.org/
Wine developer        http://wiki.winehq.org/KaiBlin
Samba team member     http://www.samba.org/samba/team/
--
Will code for cotton.

[PATCH] AcceptEx Impelementation

[Scott Lindeneau]

Date: Mon, 4 Aug 2008 05:20:43 -0700
From: "Dan Kegel" <dank(a)kegel.com>
> 2) port some existing accept() conformance tests from ws2_32/tests/sock.c
> to use AcceptEx(), add a bit to check the special new features,
> and make sure they pass on Windows.

On Sun, Aug 10, 2008 at 4:16 PM, Kai Blin <kai.blin(a)gmail.com> wrote:
> The indentation common in socket.c is either a two-space indent or a
> four-space indent. Please don't add new code with an eight-space indent.
>
> Also, the common convention for ifs is
> if (cond)
> {
> and not
> if(cond){

Thanks guys. I only work on this stuff every so often so I apologize
for the slow replies.  I think I've made the changes kai suggested and
I have been working on the Tests that Dan wants.... but for the life
of me I can't get some parts of it to work.... Theres just too much
code (and repeated code) to sift through. The conformance tests break
the AcceptEx code as it is written, (as far as I know) and thus I know
the code isn't correct (and i Don't have any windows machines to test
the code on)...  I will resubmit the patches again in a couple of days
if I can figure out what is wrong.

If anyone can tell me how to check to see if there are connections
pending on a sockets listen backlog (in the wineserver) and how to
post that information into the wineserver main poll loop, im all ears.
The wineserver isn't correctly posting these events.
~Scott

[PATCH] AcceptEx Impelementation

[Dan Kegel]

Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> i Don't have any windows machines to test the code on

That's a real problem.   You can't be sure the tests you're
writing are good unless they pass on a windows machine.

That said, simple tests are better than no tests,
and if you can find somebody to run your tests for
you on windows, that's good enough.  Just don't
post them to wine-patches until they pass on Windows.
> If anyone can tell me how to check to see if there are connections
> pending on a sockets listen backlog (in the wineserver) and how to
> post that information into the wineserver main poll loop, im all ears.

Can't you just see if accept() or moral equivalent grabs
a connection?
- Dan

[PATCH] AcceptEx Impelementation

[Scott Lindeneau]

On Tue, Aug 19, 2008 at 9:15 AM, Dan Kegel <dank(a)kegel.com> wrote:
> Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> > i Don't have any windows machines to test the code on
>
> That's a real problem.   You can't be sure the tests you're
> writing are good unless they pass on a windows machine.

Is an emulated environment using qemu good enough?
> Can't you just see if accept() or moral equivalent grabs
> a connection?
> - Dan

That's a good idea, but I think the wineserver already knows about it,
and its clobbering that information somewhere accidentally. (Also, it
means further complicating the accept function in the wineserver...
and complicated things make life hard for the next person who has to
tinker with it. If it's the only way to do it, I will do it.)
- Scott

[PATCH] AcceptEx Impelementation

[Austin English]

On Wed, Aug 20, 2008 at 7:48 AM, Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> On Tue, Aug 19, 2008 at 9:15 AM, Dan Kegel <dank(a)kegel.com> wrote:
> > Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> > > i Don't have any windows machines to test the code on
> >
> > That's a real problem.   You can't be sure the tests you're
> > writing are good unless they pass on a windows machine.
>
> Is an emulated environment using qemu good enough?

Yes, or send a request for someone to test to wine-devel.

[PATCH] AcceptEx Impelementation

[Dan Kegel]

Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> Is an emulated environment using qemu good enough?

Yes, for networking stuff it'd be just fine.
> > > If anyone can tell me how to check to see if there are connections
> > > pending on a sockets listen backlog (in the wineserver) and how to
> > > post that information into the wineserver main poll loop, im all ears.
> > > The wineserver isn't correctly posting these events.
> >
> > Can't you just see if accept() or moral equivalent grabs
> > a connection?
>
> That's a good idea, but I think the wineserver already knows about it,
> and its clobbering that information somewhere accidentally. (Also, it
> means further complicating the accept function in the wineserver...

??  Not sure what you're talking about there.  Your tests
should not try to get at any hidden Wine info.  They
should simply use plain old Windows networking calls
that should not complicate the wineserver.
- Dan

[PATCH] AcceptEx Impelementation

[Scott Lindeneau]

> ??  Not sure what you're talking about there.  Your tests
> should not try to get at any hidden Wine info.  They
> should simply use plain old Windows networking calls
> that should not complicate the wineserver.
> - Dan

Ah. My apologies. My question was related to writing the
implementation of acceptex. I have recently solved the problem and the
question is now moot. The problem was related to a bug in the
wineserver async implementation that appeared when a file descriptor
handled its own polling events (like sockets) and garbage collection.
Fixing the bug fixes the problem i was having earlier. Should I
include those fixes in a separate patch or can I incorporate them into
the acceptex patch? (Correct AcceptEx implementation relies on it).

[PATCH] AcceptEx Impelementation

[Juan Lang]

> Ah. My apologies. My question was related to writing the
> implementation of acceptex. I have recently solved the problem and the
> question is now moot. The problem was related to a bug in the
> wineserver async implementation that appeared when a file descriptor
> handled its own polling events (like sockets) and garbage collection.
> Fixing the bug fixes the problem i was having earlier. Should I
> include those fixes in a separate patch or can I incorporate them into
> the acceptex patch? (Correct AcceptEx implementation relies on it).

Those fixes should be a separate patch, perhaps one in a series of patches.
--Juan

[PATCH] AcceptEx Impelementation

[Dan Kegel]

Scott Lindeneau <slindeneau(a)gmail.com> wrote:
> The problem was related to a bug in the
> wineserver async implementation that appeared when a file descriptor
> handled its own polling events (like sockets) and garbage collection.
> Fixing the bug fixes the problem i was having earlier. Should I
> include those fixes in a separate patch or can I incorporate them into
> the acceptex patch? (Correct AcceptEx implementation relies on it).

If you can write a conformance test to expose the bug,
then please send that and a patch for the fix, e.g.
as the first two patches in a series:
  1/4 async test
  2/4 async fix
  3/4 acceptex test
  4/4 acceptex fix
If no separate test is possible, just fold it in to the AcceptEx patches.
Thanks!
- Dan

[PATCH] AcceptEx Impelementation

[Scott Lindeneau]

The async bug was revealed as I was writing the acceptex tests. The
only observable effect was a thread hanging because it never gets
notified. I don't know how/where to write conformance tests for the
wineserver core.
> If you can write a conformance test to expose the bug,
> then please send that and a patch for the fix, e.g.
> as the first two patches in a series:
>  1/4 async test
>  2/4 async fix
>  3/4 acceptex test
>  4/4 acceptex fix
> If no separate test is possible, just fold it in to the AcceptEx patches.
> Thanks!
> - Dan