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.
