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
