[ https://issues.apache.org/jira/browse/ZOOKEEPER-1029?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15050129#comment-15050129 ]
Raul Gutierrez Segales commented on ZOOKEEPER-1029: --------------------------------------------------- [~fpj]: a few comments: * in lock_reconfig/unlock_reconfig and enter_critical/leave_critical if the the adaptor is not initialized we return 0 (lock or unlock succeeded) - is this actually desired? If so, maybe add a comment explaining why this is so? * in: {code} - lock_completion_list(&zh->sent_requests); - tmp_list = zh->sent_requests; - zh->sent_requests.head = 0; - zh->sent_requests.last = 0; - unlock_completion_list(&zh->sent_requests); - while (tmp_list.head) { .... + if (lock_completion_list(&zh->sent_requests) == 0) { + tmp_list = zh->sent_requests; + zh->sent_requests.head = 0; + zh->sent_requests.last = 0; + unlock_completion_list(&zh->sent_requests); + while (tmp_list.head) { {code} we are now iterating over tmp_list.head *only if* zh->requests was locked - do we need to couple them? Or should it be: {code} + if (lock_completion_list(&zh->sent_requests) == 0) { + tmp_list = zh->sent_requests; + zh->sent_requests.head = 0; + zh->sent_requests.last = 0; + unlock_completion_list(&zh->sent_requests); + } + while (tmp_list.head) { + ... {code} ? * similarly, in: {code} - a_list.completion = NULL; - a_list.next = NULL; - zoo_lock_auth(zh); ... + if (zoo_lock_auth(zh) == 0) { + a_list.completion = NULL; + a_list.next = NULL; + + get_auth_completions(&zh->auth_h, &a_list); + zoo_unlock_auth(zh); {code} setting a_list.completion = NULL and a_list.next = NULL didn't previously depend on obtaining zoo_lock_auth.. but now they do. Again, should it just be: {code} + a_list.completion = NULL; + a_list.next = NULL; + if (zoo_lock_auth(zh) == 0) { + get_auth_completions(&zh->auth_h, &a_list); + zoo_unlock_auth(zh); + } {code} ? > C client bug in zookeeper_init (if bad hostname is given) > --------------------------------------------------------- > > Key: ZOOKEEPER-1029 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1029 > Project: ZooKeeper > Issue Type: Bug > Components: c client > Affects Versions: 3.3.2, 3.4.6, 3.5.0 > Reporter: Dheeraj Agrawal > Assignee: Flavio Junqueira > Priority: Blocker > Fix For: 3.4.7, 3.5.2, 3.6.0 > > Attachments: ZOOKEEPER-1029-3.4.patch, ZOOKEEPER-1029-3.4.patch, > ZOOKEEPER-1029-3.4.patch, ZOOKEEPER-1029-3.4.patch, ZOOKEEPER-1029-3.4.patch, > ZOOKEEPER-1029-3.4.patch, ZOOKEEPER-1029-3.5.patch > > > If you give invalid hostname to zookeeper_init method, it's not able to > resolve it, and it tries to do the cleanup (free buffer/completion lists/etc) > . The adaptor_init() is not called for this code path, so the lock,cond > variables (for adaptor, completion lists) are not initialized. > As part of the cleanup it's trying to clean up some buffers and acquires > locks and unlocks (where the locks have not yet been initialized, so > unlocking fails) > lock_completion_list(&zh->sent_requests); - pthread_mutex/cond not > initialized > tmp_list = zh->sent_requests; > zh->sent_requests.head = 0; > zh->sent_requests.last = 0; > unlock_completion_list(&zh->sent_requests); trying to broadcast here > on uninitialized cond > It should do error checking to see if locking succeeds before unlocking it. > If Locking fails, then appropriate error handling has to be done. -- This message was sent by Atlassian JIRA (v6.3.4#6332)