[sheepdog] [RFC PATCH 1/2] make list iterator functions safe against removal of entry

MORITA Kazutaka morita.kazutaka at gmail.com
Mon Aug 19 10:36:16 CEST 2013


From: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>

Removing entries in non-safe list iterators causes severe bugs and
they are usually difficult to find.  This makes all the iterator
functions safe against removal of list entries in exchange for a very
small overhead.

Signed-off-by: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
---
 dog/farm/farm.c           |    4 ++--
 dog/farm/object_tree.c    |    4 ++--
 dog/treeview.c            |    4 ++--
 include/list.h            |   47 +++++++++++++++------------------------------
 sheep/cluster/zookeeper.c |    4 ++--
 sheep/group.c             |   10 +++++-----
 sheep/object_cache.c      |   16 +++++++--------
 sheep/object_list_cache.c |    4 ++--
 sheep/request.c           |   20 +++++++++----------
 9 files changed, 49 insertions(+), 64 deletions(-)

diff --git a/dog/farm/farm.c b/dog/farm/farm.c
index 1969927..14f6278 100644
--- a/dog/farm/farm.c
+++ b/dog/farm/farm.c
@@ -101,8 +101,8 @@ static int create_active_vdis(void)
 
 static void free_vdi_list(void)
 {
-	struct vdi_entry *vdi, *next;
-	list_for_each_entry_safe(vdi, next, &last_vdi_list, list)
+	struct vdi_entry *vdi;
+	list_for_each_entry(vdi, &last_vdi_list, list)
 		free(vdi);
 }
 
diff --git a/dog/farm/object_tree.c b/dog/farm/object_tree.c
index 00cad2d..93ee9a1 100644
--- a/dog/farm/object_tree.c
+++ b/dog/farm/object_tree.c
@@ -91,8 +91,8 @@ void object_tree_print(void)
 
 void object_tree_free(void)
 {
-	struct object_tree_entry *entry, *next;
-	list_for_each_entry_safe(entry, next, &tree.list, list)
+	struct object_tree_entry *entry;
+	list_for_each_entry(entry, &tree.list, list)
 		free(entry);
 
 	free(cached_entry);
diff --git a/dog/treeview.c b/dog/treeview.c
index baf0f00..fe7c7e6 100644
--- a/dog/treeview.c
+++ b/dog/treeview.c
@@ -87,9 +87,9 @@ void add_vdi_tree(const char *name, const char *label, uint32_t vid,
 
 static void compaction(struct vdi_tree *parent)
 {
-	struct vdi_tree *vdi, *e, *new_parent;
+	struct vdi_tree *vdi, *new_parent;
 
-	list_for_each_entry_safe(vdi, e, &parent->children, siblings) {
+	list_for_each_entry(vdi, &parent->children, siblings) {
 		new_parent = find_vdi(root, vdi->pvid, vdi->name);
 		if (new_parent && parent != new_parent)
 			list_move_tail(&vdi->siblings, &new_parent->children);
diff --git a/include/list.h b/include/list.h
index 4b9d9a7..e37b37e 100644
--- a/include/list.h
+++ b/include/list.h
@@ -36,18 +36,17 @@ static inline int list_empty(const struct list_head *head)
 	container_of(ptr, type, member)
 
 #define list_for_each(pos, head) \
-	for (pos = (head)->next; pos != (head); pos = pos->next)
+	pos = (head)->next;					\
+	for (typeof(pos) __n = pos->next; pos != (head);	\
+	     pos = __n, __n = pos->next)
 
 #define list_for_each_entry(pos, head, member)				\
-	for (pos = list_entry((head)->next, typeof(*pos), member);	\
+	pos = list_entry((head)->next, typeof(*pos), member);		\
+	for (typeof(pos) __n = list_entry(pos->member.next, typeof(*pos), \
+					  member);			\
 	     &pos->member != (head);					\
-	     pos = list_entry(pos->member.next, typeof(*pos), member))
-
-#define list_for_each_entry_safe(pos, n, head, member)			\
-	for (pos = list_entry((head)->next, typeof(*pos), member),	\
-		n = list_entry(pos->member.next, typeof(*pos), member);	\
-	     &pos->member != (head);					\
-	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
+	     pos = __n, __n = list_entry(__n->member.next, typeof(*__n), \
+					 member))
 
 static inline void __list_add(struct list_head *new,
 			      struct list_head *prev,
@@ -218,11 +217,10 @@ static inline void hlist_add_after(struct hlist_node *n,
 
 #define hlist_entry(ptr, type, member) container_of(ptr, type, member)
 
-#define hlist_for_each(pos, head) \
-	for (pos = (head)->first; pos ; pos = pos->next)
-
-#define hlist_for_each_safe(pos, n, head) \
-	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); pos = n)
+#define hlist_for_each(pos, head)				\
+	pos = (head)->first;					\
+	for (typeof(pos) __n; pos && ({ __n = pos->next; 1; });	\
+	     pos = __n)						\
 
 /*
  * hlist_for_each_entry - iterate over list of given type
@@ -232,24 +230,11 @@ static inline void hlist_add_after(struct hlist_node *n,
  * @member:     the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry(tpos, pos, head, member)			\
-	for (pos = (head)->first;					\
-	     pos && ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
-	     pos = pos->next)
-
-/*
- * hlist_for_each_entry_safe - iterate over list of given type safe against
- * removal of list entry
- * @tpos:       the type * to use as a loop cursor.
- * @pos:        the &struct hlist_node to use as a loop cursor.
- * @n:          another &struct hlist_node to use as temporary storage
- * @head:       the head for your list.
- * @member:     the name of the hlist_node within the struct.
- */
-#define hlist_for_each_entry_safe(tpos, pos, n, head, member)		\
-	for (pos = (head)->first;					\
-	     pos && ({ n = pos->next; 1; }) &&				\
+	pos = (head)->first;						\
+	for (typeof(pos) __n;						\
+	     pos && ({ __n = pos->next; 1; }) &&			\
 		     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
-	     pos = n)
+	     pos = __n)
 
 void list_sort(void *priv, struct list_head *head,
 	       int (*cmp)(void *priv, struct list_head *a,
diff --git a/sheep/cluster/zookeeper.c b/sheep/cluster/zookeeper.c
index 5088968..8a9804d 100644
--- a/sheep/cluster/zookeeper.c
+++ b/sheep/cluster/zookeeper.c
@@ -924,9 +924,9 @@ static void kick_block_event(void)
 
 static void block_event_list_del(struct zk_node *n)
 {
-	struct zk_node *ev, *t;
+	struct zk_node *ev;
 
-	list_for_each_entry_safe(ev, t, &zk_block_list, list) {
+	list_for_each_entry(ev, &zk_block_list, list) {
 		if (node_eq(&ev->node, &n->node)) {
 			list_del(&ev->list);
 			free(ev);
diff --git a/sheep/group.c b/sheep/group.c
index acd6f41..587e982 100644
--- a/sheep/group.c
+++ b/sheep/group.c
@@ -774,12 +774,12 @@ static int send_join_request(struct sd_node *ent)
 
 static void requeue_cluster_request(void)
 {
-	struct request *req, *p;
+	struct request *req;
 	struct vdi_op_message *msg;
 	size_t size;
 
-	list_for_each_entry_safe(req, p, main_thread_get(pending_notify_list),
-				 pending_list) {
+	list_for_each_entry(req, main_thread_get(pending_notify_list),
+			    pending_list) {
 		/*
 		 * ->notify() was called and succeeded but after that
 		 * this node session-timeouted and sd_notify_handler
@@ -794,8 +794,8 @@ static void requeue_cluster_request(void)
 		free(msg);
 	}
 
-	list_for_each_entry_safe(req, p, main_thread_get(pending_block_list),
-				 pending_list) {
+	list_for_each_entry(req, main_thread_get(pending_block_list),
+			    pending_list) {
 		switch (req->status) {
 		case REQUEST_INIT:
 			/* this request has never been executed, re-queue it */
diff --git a/sheep/object_cache.c b/sheep/object_cache.c
index 1f8822e..4d22e28 100644
--- a/sheep/object_cache.c
+++ b/sheep/object_cache.c
@@ -538,12 +538,12 @@ out:
 #define HIGH_WATERMARK (sys->object_cache_size * 9 / 10)
 static void do_reclaim_object(struct object_cache *oc)
 {
-	struct object_cache_entry *entry, *t;
+	struct object_cache_entry *entry;
 	uint64_t oid;
 	uint32_t cap;
 
 	write_lock_cache(oc);
-	list_for_each_entry_safe(entry, t, &oc->lru_head, lru_list) {
+	list_for_each_entry(entry, &oc->lru_head, lru_list) {
 		oid = idx_to_oid(oc->vid, entry_idx(entry));
 		if (entry_in_use(entry)) {
 			sd_debug("%"PRIx64" is in use, skip...", oid);
@@ -927,7 +927,7 @@ static void push_object_done(struct work *work)
  */
 static int object_cache_push(struct object_cache *oc)
 {
-	struct object_cache_entry *entry, *t;
+	struct object_cache_entry *entry;
 
 	write_lock_cache(oc);
 	if (list_empty(&oc->dirty_head)) {
@@ -936,7 +936,7 @@ static int object_cache_push(struct object_cache *oc)
 	}
 
 	uatomic_set(&oc->push_count, uatomic_read(&oc->dirty_count));
-	list_for_each_entry_safe(entry, t, &oc->dirty_head, dirty_list) {
+	list_for_each_entry(entry, &oc->dirty_head, dirty_list) {
 		struct push_work *pw;
 
 		get_cache_entry(entry);
@@ -972,7 +972,7 @@ void object_cache_delete(uint32_t vid)
 {
 	struct object_cache *cache;
 	int h = hash(vid);
-	struct object_cache_entry *entry, *t;
+	struct object_cache_entry *entry;
 	char path[PATH_MAX];
 
 	cache = find_object_cache(vid, false);
@@ -985,7 +985,7 @@ void object_cache_delete(uint32_t vid)
 	sd_unlock(&hashtable_lock[h]);
 
 	write_lock_cache(cache);
-	list_for_each_entry_safe(entry, t, &cache->lru_head, lru_list) {
+	list_for_each_entry(entry, &cache->lru_head, lru_list) {
 		free_cache_entry(entry);
 		uatomic_sub(&gcache.capacity, CACHE_OBJECT_SIZE);
 	}
@@ -1391,12 +1391,12 @@ err:
 void object_cache_format(void)
 {
 	struct object_cache *cache;
-	struct hlist_node *node, *t;
+	struct hlist_node *node;
 	int i;
 
 	for (i = 0; i < HASH_SIZE; i++) {
 		struct hlist_head *head = cache_hashtable + i;
-		hlist_for_each_entry_safe(cache, node, t, head, hash) {
+		hlist_for_each_entry(cache, node, head, hash) {
 			object_cache_delete(cache->vid);
 		}
 	}
diff --git a/sheep/object_list_cache.c b/sheep/object_list_cache.c
index 1710522..f572544 100644
--- a/sheep/object_list_cache.c
+++ b/sheep/object_list_cache.c
@@ -163,7 +163,7 @@ static void objlist_deletion_work(struct work *work)
 {
 	struct objlist_deletion_work *ow =
 		container_of(work, struct objlist_deletion_work, work);
-	struct objlist_cache_entry *entry, *t;
+	struct objlist_cache_entry *entry;
 	uint32_t vid = ow->vid, entry_vid;
 
 	/*
@@ -180,7 +180,7 @@ static void objlist_deletion_work(struct work *work)
 	}
 
 	sd_write_lock(&obj_list_cache.lock);
-	list_for_each_entry_safe(entry, t, &obj_list_cache.entry_list, list) {
+	list_for_each_entry(entry, &obj_list_cache.entry_list, list) {
 		entry_vid = oid_to_vid(entry->oid);
 		if (entry_vid != vid)
 			continue;
diff --git a/sheep/request.c b/sheep/request.c
index dbb13cf..27ccb72 100644
--- a/sheep/request.c
+++ b/sheep/request.c
@@ -197,12 +197,12 @@ static bool request_in_recovery(struct request *req)
 /* Wakeup requests because of epoch mismatch */
 void wakeup_requests_on_epoch(void)
 {
-	struct request *req, *t;
+	struct request *req;
 	LIST_HEAD(pending_list);
 
 	list_splice_init(&sys->req_wait_queue, &pending_list);
 
-	list_for_each_entry_safe(req, t, &pending_list, request_list) {
+	list_for_each_entry(req, &pending_list, request_list) {
 		switch (req->rp.result) {
 		case SD_RES_OLD_NODE_VER:
 			/*
@@ -234,12 +234,12 @@ void wakeup_requests_on_epoch(void)
 /* Wakeup the requests on the oid that was previously being recoverred */
 void wakeup_requests_on_oid(uint64_t oid)
 {
-	struct request *req, *t;
+	struct request *req;
 	LIST_HEAD(pending_list);
 
 	list_splice_init(&sys->req_wait_queue, &pending_list);
 
-	list_for_each_entry_safe(req, t, &pending_list, request_list) {
+	list_for_each_entry(req, &pending_list, request_list) {
 		if (req->local_oid != oid)
 			continue;
 		sd_debug("retry %" PRIx64, req->local_oid);
@@ -250,12 +250,12 @@ void wakeup_requests_on_oid(uint64_t oid)
 
 void wakeup_all_requests(void)
 {
-	struct request *req, *n;
+	struct request *req;
 	LIST_HEAD(pending_list);
 
 	list_splice_init(&sys->req_wait_queue, &pending_list);
 
-	list_for_each_entry_safe(req, n, &pending_list, request_list) {
+	list_for_each_entry(req, &pending_list, request_list) {
 		sd_debug("%"PRIx64, req->rq.obj.oid);
 		del_requeue_request(req);
 	}
@@ -640,11 +640,11 @@ static void destroy_client(struct client_info *ci)
 
 static void clear_client_info(struct client_info *ci)
 {
-	struct request *req, *t;
+	struct request *req;
 
 	sd_debug("connection seems to be dead");
 
-	list_for_each_entry_safe(req, t, &ci->done_reqs, request_list) {
+	list_for_each_entry(req, &ci->done_reqs, request_list) {
 		list_del(&req->request_list);
 		free_request(req);
 	}
@@ -809,7 +809,7 @@ int init_unix_domain_socket(const char *dir)
 
 static void local_req_handler(int listen_fd, int events, void *data)
 {
-	struct request *req, *t;
+	struct request *req;
 	LIST_HEAD(pending_list);
 
 	if (events & EPOLLERR)
@@ -821,7 +821,7 @@ static void local_req_handler(int listen_fd, int events, void *data)
 	list_splice_init(&sys->local_req_queue, &pending_list);
 	pthread_mutex_unlock(&sys->local_req_lock);
 
-	list_for_each_entry_safe(req, t, &pending_list, request_list) {
+	list_for_each_entry(req, &pending_list, request_list) {
 		list_del(&req->request_list);
 		queue_request(req);
 	}
-- 
1.7.9.5




More information about the sheepdog mailing list