<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">

<html><body><div id="mbx-wrapper">

<div id="mbx-content" class="content" onfocus="didBecomeFirstResponder()" contenteditable="true" onblur="didBlur()">Responses inline. </div>

<div id="mbx-old-email" contenteditable="true">

<div class="mailbox_signature" style="display: block;">

<br>Best,<div>-Matt</div>

</div>

<span id="orc-full-body-initial-text" style="display: inline;"><br>On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <<a href="mailto:ken@codeweavers.com" target="_blank">ken@codeweavers.com</a>>, wrote:<br></span><blockquote class="gmail_quote">Thanks for continuing to work on this.  By no means a thorough review:

<br><br>> On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich@gmail.com> wrote:

<br><br>> +#define CACHE_SIZE  256

<br>> +#define CACHE_DEPTH 16

<br>> +static SOCKET socket_cache[CACHE_SIZE][CACHE_DEPTH];

<br>> +

<br>> +/* Cache support */

<br>> +static void add_to_cache(SOCKET s);

<br>> +static BOOL remove_from_cache(SOCKET s);

<br>> +static BOOL socket_in_cache(SOCKET s);

I'm not sure this is a "cache", per se. It's a hash table, but naming issues are fairly minor.

<br><br>

</blockquote>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Let's go with socket_table</span><div>

<br><blockquote class="gmail_quote">

<br>> @@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)

<br>> INT WINAPI WSACleanup(void)

<br>> {

<br>>     if (num_startup) {

<br>> -        num_startup--;

<br>> -        TRACE("pending cleanups: %d\n", num_startup);

<br>> +        /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */

<br>> +        if (num_startup - 1 == 0) {

<br><br>Why not "if (num_startup == 1)"?

<br><br>> +static void add_to_cache(SOCKET s)

<br>> +{

<br>> +    int index, depth;

<br>> +    SOCKET old;

<br>> +    LONG *dest;

<br>> +    index = socket_to_index(s) % CACHE_SIZE; 

If you always mod the result of socket_to_index() by CACHE_SIZE, why not do that in that function?

<br><br>

</blockquote>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Point taken. </span><div>

<br><div><blockquote class="gmail_quote">

<br>> +    for (depth = 0; depth < CACHE_DEPTH; ++depth) {

<br>> +        if (socket_cache[index][depth] == 0)

<br>> +            break;

<br>> +    }

<br>> +

<br>> +    if (depth == CACHE_DEPTH) {

<br>> +        ERR("Socket hash table collision\n");

This should exit here or you access beyond the end of socket_cache[index] just below.

<br><br>

</blockquote></div>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Good catch. </span><div>

<br><div><div><blockquote class="gmail_quote">

<br>> +    }

<br>> +

<br>> +    dest = (PLONG)&socket_cache[index][depth]; 

Don't use PLONG, just use LONG*.

<br><br>

</blockquote></div></div>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Ok</span><div>

<br><div><div><div><blockquote class="gmail_quote">

<br>> +    old = InterlockedExchange(dest, s);

<br>> +

<br>> +    if (old != 0) {

<br>> +        ERR("Socket hash table internal corruption");

Reporting corruption isn't right.  You should use InterlockedCompareExchange() with 0 as the comparand to _avoid_ corruption.  If it fails, loop back to the depth loop and try again.  The only error possible should be filling the hash table bucket.

<br><br>

</blockquote></div></div></div>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Right. Makes sense. Will fix. </span><div>

<br><div><div><div><div><blockquote class="gmail_quote">

<br>> +    }

<br>> +}

<br>> +

<br>> +static BOOL remove_from_cache(SOCKET s)

<br>> +{

<br>> +    int index,depth;

<br>> +    SOCKET old;

<br>> +    LONG *dest;

<br>> +    index = socket_to_index(s) % CACHE_SIZE;

<br>> +    for (depth = 0; depth < CACHE_DEPTH; ++depth) {

<br>> +        if (socket_cache[index][depth] == s)

<br>> +            break;

<br>> +    }

<br>> +

<br>> +    if (depth == CACHE_DEPTH) {

<br>> +        return FALSE;

<br>> +    }

<br>> +

<br>> +    dest = (PLONG)&socket_cache[index][depth];

<br>> +    old = InterlockedExchange(dest, 0);

<br>> +    return (old == s);

Under what circumstances could old not equal s?  If that can happen, then this should also use InterlockedCompareExchange(), otherwise you've removed some other socket.

<br><br>

</blockquote></div></div></div></div>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">If I fork a child, I'll inherit their sockets right? So some paranoia in the remove is needed. Unless wine doesn't work that way. I'll write it as the analog to add. </span><div>

<br><div><div><div><div><div>

<blockquote class="gmail_quote">

<br>-Ken

<br><br>

</blockquote>

<blockquote class="gmail_quote"></blockquote>

</div></div></div></div></div>

</div>

<span class="mailbox-inline-edit"></span><div></div>

<span class="mailbox-inline-edit">Thanks! I appreciate the feedback. </span><div>

<br><div><div><div><div><div><div><blockquote class="gmail_quote"><br></blockquote></div></div></div></div></div></div>

</div>

</div>

</div></body></html>