Interface Patch

Tomash Brechko tomash.brechko at gmail.com
Mon Feb 4 07:37:11 UTC 2008


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


More information about the memcached mailing list