no reply patch

Brian Aker brian at tangent.org
Tue Feb 5 03:14:21 UTC 2008


Hi!

The patch looks fine, though I think you should move this:

+    if (c->noreply) {
+        c->noreply = false;
+        conn_set_state(c, conn_read);
+        return;
+    }
+

... to being after the verbose if () since without it debugging  
problems is going to be a real pain.

One thought I had with this is that you could use the token you have  
added to the server as an additional action verb for future commands.

Cheers,
	-Brian


On Feb 3, 2008, at 11:37 PM, Tomash Brechko wrote:

> On Sun, Feb 03, 2008 at 13:33:24 -0800, Brian Aker wrote:
>> BTW if you can resend the patch for your no-op stuff I'd be happy to
>> review that.
>
> I guess you mean 'noreply' patch.  It may be found here:
>
>  http://git.openhack.ru/?p=memcached-patched.git;a=commitdiff;h=7dd54ac11da58690a64fbefcf5dcc81af4fe664b
>
>
> On Sun, Feb 03, 2008 at 16:18:53 -0800, Brian Aker wrote:
>> This is the second interface patch. It answers Tomash's questions,  
>> and
>> comes after a conversation with Dormando.
>
>
>> @@ -381,6 +380,7 @@ conn *conn_new(const int sfd, const int
>>         if (conn_add_to_freelist(c)) {
>>             conn_free(c);
>>         }
>> +        perror("event_add");
>>         return NULL;
>>     }
>
> event_add() is not a system call, perror() may or may not return
> something meaningful, depending on whether the failure happened in a
> syscall executed from event_add(), or in the event_add()'s own logic.
>
>
>> +    sfd_list= (int *)calloc(*count, sizeof(int));
>> +    if (sfd_list == NULL) {
>> +        perror("calloc()");
>> +        return NULL;
>> +    }
>> +    memset(sfd_list, -1, sizeof(int) * (*count));
>
> Likewise, malloc() and friends are not system calls, and do not set  
> errno.
>
> Back then I meant it's either calloc() (zeroing), or initialization
> with -1, or no initialization at all (and after your explanation only
> "-1" seemed to be the right thing to do).  So no need to call calloc()
> and then overwriting zeroes with -1, you may use malloc().
>
> On a larger scale, you are storing the array size now along with the
> array.  Instead of using 'success' counter in the binding code, you
> could store in *count the number of _initialized_ sockets.  Then the
> whole thing will be
>
>  - malloc() the array
>
>  - initialize continuous number of elements from the start, set
>    *count to the number of initialized elements, leaving the rest
>    uninitialized.
>
> With this, no initialization and later tests against -1 will be
> required, you will always know that first *count elems are valid.
>
>
>> +        if (bind(sfd, next->ai_addr, next->ai_addrlen) == -1) {
>> +            if (errno != EADDRINUSE) {
>> +                int *sfd_ptr;
>> +                int x;
>> +
>> +                perror("bind()");
>> +                for (sfd_ptr= sfd_list, x = 0; x < *count; sfd_ptr+ 
>> +) {
>> +                    if (*sfd_ptr > -1 )
>> +                        close(sfd);
>> +                }
>
> Looks like an infinite loop (x is never increased), and wrong sfd is
> closed.  Instead it can be
>
>               if (errno != EADDRINUSE) {
>                   perror("bind()");
>                   ++sfd_ptr;
>                   while (sfd_ptr != sfd_list) {
>                       --sfd_ptr;
>                       close(*sfd_ptr);
>                   }
>
> The trick with initial increment of sfd_ptr, and decrement before the
> close() is not a mistake.  This is required because pointer to the
> element one past the last is defined, while to one before the first is
> not.
>
>
>> +              for (sfd_ptr= sfd_list, x = 0; x < *count; sfd_ptr+ 
>> +) {
>> +                  if (*sfd_ptr > -1 )
>> +                      close(sfd);
>> +              }
>
> Copy-pastes are evil ;).
>
>
> -- 
>   Tomash Brechko

--
_______________________________________________________
Brian "Krow" Aker, brian at tangent.org
Seattle, Washington
http://krow.net/                     <-- Me
http://tangent.org/                <-- Software
http://exploitseattle.com/    <-- Fun
_______________________________________________________
You can't grep a dead tree.




More information about the memcached mailing list