[Qemu-devel] [PATCH 0/2] Use TAILQ for breakpoints

[Qemu-devel] [PATCH 0/2] Use TAILQ for breakpoints

[Jan Kiszka]

As requested by Anthony, here is a follow-up series to switch open-coded
list management for breakpoints and watchpoints to sys-queue.h, TAILQ.
Refactoring had the nice effect of finding a bug in the original
version...

Find the patches also at git://git.kiszka.org/qemu.git gdb-queue

Jan

--
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 2/2] Use sys-queue.h for break/watchpoint managment

[Jan Kiszka]

This switches cpu_break/watchpoint_* to TAILQ wrappers, simplifying the
code and also fixing a use after release issue in
cpu_break/watchpoint_remove_all.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 cpu-defs.h               |    9 +++--
 cpu-exec.c               |    2 +
 exec.c                   |   84 +++++++++++++---------------------------------
 target-alpha/translate.c |    4 +-
 target-arm/translate.c   |    4 +-
 target-cris/translate.c  |    4 +-
 target-i386/helper.c     |    2 +
 target-i386/translate.c  |    4 +-
 target-m68k/translate.c  |    4 +-
 target-mips/translate.c  |    4 +-
 target-ppc/translate.c   |    4 +-
 target-sh4/translate.c   |    4 +-
 target-sparc/translate.c |    4 +-
 13 files changed, 49 insertions(+), 84 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 069d312..6eee881 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -28,6 +28,7 @@
 #include <setjmp.h>
 #include <inttypes.h>
 #include "osdep.h"
+#include "sys-queue.h"
 
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
@@ -146,14 +147,14 @@ struct KVMState;
 typedef struct CPUBreakpoint {
     target_ulong pc;
     int flags; /* BP_* */
-    struct CPUBreakpoint *prev, *next;
+    TAILQ_ENTRY(CPUBreakpoint) entry;
 } CPUBreakpoint;
 
 typedef struct CPUWatchpoint {
     target_ulong vaddr;
     target_ulong len_mask;
     int flags; /* BP_* */
-    struct CPUWatchpoint *prev, *next;
+    TAILQ_ENTRY(CPUWatchpoint) entry;
 } CPUWatchpoint;
 
 #define CPU_TEMP_BUF_NLONGS 128
@@ -188,10 +189,10 @@ typedef struct CPUWatchpoint {
                                                                         \
     /* from this point: preserved by CPU reset */                       \
     /* ice debug support */                                             \
-    CPUBreakpoint *breakpoints;                                         \
+    TAILQ_HEAD(breakpoints_head, CPUBreakpoint) breakpoints;            \
     int singlestep_enabled;                                             \
                                                                         \
-    CPUWatchpoint *watchpoints;                                         \
+    TAILQ_HEAD(watchpoints_head, CPUWatchpoint) watchpoints;            \
     CPUWatchpoint *watchpoint_hit;                                      \
                                                                         \
     struct GDBRegisterState *gdb_regs;                                  \
diff --git a/cpu-exec.c b/cpu-exec.c
index 8d86527..7977579 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -198,7 +198,7 @@ static void cpu_handle_debug_exception(CPUState *env)
     CPUWatchpoint *wp;
 
     if (!env->watchpoint_hit)
-        for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+        TAILQ_FOREACH(wp, &env->watchpoints, entry)
             wp->flags &= ~BP_WATCHPOINT_HIT;
 
     if (debug_excp_handler)
diff --git a/exec.c b/exec.c
index 4d39eaa..edeac23 100644
--- a/exec.c
+++ b/exec.c
@@ -537,6 +537,8 @@ void cpu_exec_init(CPUState *env)
         cpu_index++;
     }
     env->cpu_index = cpu_index;
+    TAILQ_INIT(&env->breakpoints);
+    TAILQ_INIT(&env->watchpoints);
     *penv = env;
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
     register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
@@ -1302,7 +1304,7 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
                           int flags, CPUWatchpoint **watchpoint)
 {
     target_ulong len_mask = ~(len - 1);
-    CPUWatchpoint *wp, *prev_wp;
+    CPUWatchpoint *wp;
 
     /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
     if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
@@ -1319,25 +1321,10 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
     wp->flags = flags;
 
     /* keep all GDB-injected watchpoints in front */
-    if (!(flags & BP_GDB) && env->watchpoints) {
-        prev_wp = env->watchpoints;
-        while (prev_wp->next != NULL && (prev_wp->next->flags & BP_GDB))
-            prev_wp = prev_wp->next;
-    } else {
-        prev_wp = NULL;
-    }
-
-    /* Insert new watchpoint */
-    if (prev_wp) {
-        wp->next = prev_wp->next;
-        prev_wp->next = wp;
-    } else {
-        wp->next = env->watchpoints;
-        env->watchpoints = wp;
-    }
-    if (wp->next)
-        wp->next->prev = wp;
-    wp->prev = prev_wp;
+    if (flags & BP_GDB)
+        TAILQ_INSERT_HEAD(&env->watchpoints, wp, entry);
+    else
+        TAILQ_INSERT_TAIL(&env->watchpoints, wp, entry);
 
     tlb_flush_page(env, addr);
 
@@ -1353,7 +1340,7 @@ int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
     target_ulong len_mask = ~(len - 1);
     CPUWatchpoint *wp;
 
-    for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+    TAILQ_FOREACH(wp, &env->watchpoints, entry) {
         if (addr == wp->vaddr && len_mask == wp->len_mask
                 && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
             cpu_watchpoint_remove_by_ref(env, wp);
@@ -1366,12 +1353,7 @@ int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
 /* Remove a specific watchpoint by reference.  */
 void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint)
 {
-    if (watchpoint->next)
-        watchpoint->next->prev = watchpoint->prev;
-    if (watchpoint->prev)
-        watchpoint->prev->next = watchpoint->next;
-    else
-        env->watchpoints = watchpoint->next;
+    TAILQ_REMOVE(&env->watchpoints, watchpoint, entry);
 
     tlb_flush_page(env, watchpoint->vaddr);
 
@@ -1381,11 +1363,12 @@ void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint)
 /* Remove all matching watchpoints.  */
 void cpu_watchpoint_remove_all(CPUState *env, int mask)
 {
-    CPUWatchpoint *wp;
+    CPUWatchpoint *wp, *next;
 
-    for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+    TAILQ_FOREACH_SAFE(wp, &env->watchpoints, entry, next) {
         if (wp->flags & mask)
             cpu_watchpoint_remove_by_ref(env, wp);
+    }
 }
 
 /* Add a breakpoint.  */
@@ -1393,7 +1376,7 @@ int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
                           CPUBreakpoint **breakpoint)
 {
 #if defined(TARGET_HAS_ICE)
-    CPUBreakpoint *bp, *prev_bp;
+    CPUBreakpoint *bp;
 
     bp = qemu_malloc(sizeof(*bp));
     if (!bp)
@@ -1403,25 +1386,10 @@ int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
     bp->flags = flags;
 
     /* keep all GDB-injected breakpoints in front */
-    if (!(flags & BP_GDB) && env->breakpoints) {
-        prev_bp = env->breakpoints;
-        while (prev_bp->next != NULL && (prev_bp->next->flags & BP_GDB))
-            prev_bp = prev_bp->next;
-    } else {
-        prev_bp = NULL;
-    }
-
-    /* Insert new breakpoint */
-    if (prev_bp) {
-        bp->next = prev_bp->next;
-        prev_bp->next = bp;
-    } else {
-        bp->next = env->breakpoints;
-        env->breakpoints = bp;
-    }
-    if (bp->next)
-        bp->next->prev = bp;
-    bp->prev = prev_bp;
+    if (flags & BP_GDB)
+        TAILQ_INSERT_HEAD(&env->breakpoints, bp, entry);
+    else
+        TAILQ_INSERT_TAIL(&env->breakpoints, bp, entry);
 
     breakpoint_invalidate(env, pc);
 
@@ -1439,7 +1407,7 @@ int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags)
 #if defined(TARGET_HAS_ICE)
     CPUBreakpoint *bp;
 
-    for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+    TAILQ_FOREACH(bp, &env->breakpoints, entry) {
         if (bp->pc == pc && bp->flags == flags) {
             cpu_breakpoint_remove_by_ref(env, bp);
             return 0;
@@ -1455,12 +1423,7 @@ int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags)
 void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint)
 {
 #if defined(TARGET_HAS_ICE)
-    if (breakpoint->next)
-        breakpoint->next->prev = breakpoint->prev;
-    if (breakpoint->prev)
-        breakpoint->prev->next = breakpoint->next;
-    else
-        env->breakpoints = breakpoint->next;
+    TAILQ_REMOVE(&env->breakpoints, breakpoint, entry);
 
     breakpoint_invalidate(env, breakpoint->pc);
 
@@ -1472,11 +1435,12 @@ void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint)
 void cpu_breakpoint_remove_all(CPUState *env, int mask)
 {
 #if defined(TARGET_HAS_ICE)
-    CPUBreakpoint *bp;
+    CPUBreakpoint *bp, *next;
 
-    for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+    TAILQ_FOREACH_SAFE(bp, &env->breakpoints, entry, next) {
         if (bp->flags & mask)
             cpu_breakpoint_remove_by_ref(env, bp);
+    }
 #endif
 }
 
@@ -1999,7 +1963,7 @@ int tlb_set_page_exec(CPUState *env, target_ulong vaddr,
     code_address = address;
     /* Make accesses to pages with watchpoints go via the
        watchpoint trap routines.  */
-    for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+    TAILQ_FOREACH(wp, &env->watchpoints, entry) {
         if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
             iotlb = io_mem_watch + paddr;
             /* TODO: The memory case can be optimized by not trapping
@@ -2546,7 +2510,7 @@ static void check_watchpoint(int offset, int len_mask, int flags)
         return;
     }
     vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
-    for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+    TAILQ_FOREACH(wp, &env->watchpoints, entry) {
         if ((vaddr == (wp->vaddr & len_mask) ||
              (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
             wp->flags |= BP_WATCHPOINT_HIT;
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 3b90f62..7a0e54f 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2363,8 +2363,8 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
 
     gen_icount_start();
     for (ret = 0; ret == 0;) {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == ctx.pc) {
                     gen_excp(&ctx, EXCP_DEBUG, 0);
                     break;
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 54eb067..f984de7 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8677,8 +8677,8 @@ static inline void gen_intermediate_code_internal(CPUState *env,
         }
 #endif
 
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
                     gen_set_condexec(dc);
                     gen_set_pc_im(dc->pc);
diff --git a/target-cris/translate.c b/target-cris/translate.c
index ac258a9..242ef9c 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3189,8 +3189,8 @@ static void check_breakpoint(CPUState *env, DisasContext *dc)
 {
 	CPUBreakpoint *bp;
 
-	if (unlikely(env->breakpoints)) {
-		for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+	if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+		TAILQ_FOREACH(bp, &env->breakpoints, entry) {
 			if (bp->pc == dc->pc) {
 				cris_evaluate_flags (dc);
 				tcg_gen_movi_tl(env_pc, dc->pc);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2a61cb0..037540d 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1364,7 +1364,7 @@ static void breakpoint_handler(CPUState *env)
                 cpu_resume_from_signal(env, NULL);
         }
     } else {
-        for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+        TAILQ_FOREACH(bp, &env->breakpoints, entry)
             if (bp->pc == env->eip) {
                 if (bp->flags & BP_CPU) {
                     check_hw_breakpoints(env, 1);
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0de238b..612811b 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7606,8 +7606,8 @@ static inline void gen_intermediate_code_internal(CPUState *env,
 
     gen_icount_start();
     for(;;) {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == pc_ptr) {
                     gen_debug(dc, pc_ptr - dc->cs_base);
                     break;
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index a14f6c5..5c27a7e 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2999,8 +2999,8 @@ gen_intermediate_code_internal(CPUState *env, TranslationBlock *tb,
     do {
         pc_offset = dc->pc - pc_start;
         gen_throws_exception = NULL;
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
                     gen_exception(dc, dc->pc, EXCP_DEBUG);
                     dc->is_jmp = DISAS_JUMP;
diff --git a/target-mips/translate.c b/target-mips/translate.c
index cc7e71c..418b9ef 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8286,8 +8286,8 @@ gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
 #endif
     gen_icount_start();
     while (ctx.bstate == BS_NONE) {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == ctx.pc) {
                     save_cpu_state(&ctx, 1);
                     ctx.bstate = BS_BRANCH;
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 085c7a1..a294b42 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -7225,8 +7225,8 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
     gen_icount_start();
     /* Set env in case of segfault during code fetch */
     while (ctx.exception == POWERPC_EXCP_NONE && gen_opc_ptr < gen_opc_end) {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == ctx.nip) {
                     gen_update_nip(&ctx, ctx.nip);
                     gen_op_debug();
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index eb3366c..6da2154 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1837,8 +1837,8 @@ gen_intermediate_code_internal(CPUState * env, TranslationBlock * tb,
         max_insns = CF_COUNT_MASK;
     gen_icount_start();
     while (ctx.bstate == BS_NONE && gen_opc_ptr < gen_opc_end) {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (ctx.pc == bp->pc) {
 		    /* We have hit a breakpoint - make sure PC is up-to-date */
 		    tcg_gen_movi_i32(cpu_pc, ctx.pc);
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index e94e3c5..07b2624 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4816,8 +4816,8 @@ static inline void gen_intermediate_code_internal(TranslationBlock * tb,
         max_insns = CF_COUNT_MASK;
     gen_icount_start();
     do {
-        if (unlikely(env->breakpoints)) {
-            for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+        if (unlikely(!TAILQ_EMPTY(&env->breakpoints))) {
+            TAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
                     if (dc->pc != pc_start)
                         save_state(dc, cpu_cond);

[Qemu-devel] [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Jan Kiszka]

Add TAILQ iterator that allows to safely remove elements while walking
the list.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 sys-queue.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sys-queue.h b/sys-queue.h
index 3d0773e..b92a77f 100644
--- a/sys-queue.h
+++ b/sys-queue.h
@@ -210,6 +210,12 @@ struct {                                                                \
                 (var);                                                  \
                 (var) = ((var)->field.tqe_next))
 
+#define TAILQ_FOREACH_SAFE(var, head, field, next_var)                  \
+        for ((var) = ((head)->tqh_first);                               \
+                (var) ? ({ (next_var) = ((var)->field.tqe_next); 1; })  \
+                      : 0;                                              \
+                (var) = (next_var))
+
 #define TAILQ_FOREACH_REVERSE(var, head, headname, field)               \
         for ((var) = (*(((struct headname *)((head)->tqh_last))->tqh_last));    \
                 (var);                                                  \

Re: [Qemu-devel] [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Anthony Liguori]

Jan Kiszka wrote:
> Add TAILQ iterator that allows to safely remove elements while walking
> the list.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
>  sys-queue.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/sys-queue.h b/sys-queue.h
> index 3d0773e..b92a77f 100644
> --- a/sys-queue.h
> +++ b/sys-queue.h
> @@ -210,6 +210,12 @@ struct {                                                                \
>                  (var);                                                  \
>                  (var) = ((var)->field.tqe_next))
>  
> +#define TAILQ_FOREACH_SAFE(var, head, field, next_var)                  \
> +        for ((var) = ((head)->tqh_first);                               \
> +                (var) ? ({ (next_var) = ((var)->field.tqe_next); 1; })  \
> +                      : 0;                                              \
> +                (var) = (next_var))
>   

So the Linux implementation of this is:

#define list_for_each_safe(pos, n, head) \
    for (pos = (head)->next, n = pos->next; pos != (head); \
        pos = n, n = pos->next)

I'd prefer something similar:

#define TAILQ_FOREACH_SAFE(var, head, field, next_var)
             for (var = (head)->tqh_first, next_var = var->field.tqe_next;
                   var;
                   var = next_var, next_var = var->field.tqe_next)

As it's functionally equivalent and avoids using a GCC-ism ({}).

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Use sys-queue.h for break/watchpoint managment

[Anthony Liguori]

Jan Kiszka wrote:
> This switches cpu_break/watchpoint_* to TAILQ wrappers, simplifying the
> code and also fixing a use after release issue in
> cpu_break/watchpoint_remove_all.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

Really nice cleanup.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Add TAILQ iterator that allows to safely remove elements while walking
>> the list.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  sys-queue.h |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/sys-queue.h b/sys-queue.h
>> index 3d0773e..b92a77f 100644
>> --- a/sys-queue.h
>> +++ b/sys-queue.h
>> @@ -210,6 +210,12 @@ struct
>> {                                                                \
>>                 
>> (var);                                                  \
>>                  (var) = ((var)->field.tqe_next))
>>  
>> +#define TAILQ_FOREACH_SAFE(var, head, field,
>> next_var)                  \
>> +        for ((var) =
>> ((head)->tqh_first);                               \
>> +                (var) ? ({ (next_var) = ((var)->field.tqe_next); 1;
>> })  \
>> +                      :
>> 0;                                              \
>> +                (var) = (next_var))
>>   
> 
> So the Linux implementation of this is:
> 
> #define list_for_each_safe(pos, n, head) \
>    for (pos = (head)->next, n = pos->next; pos != (head); \
>        pos = n, n = pos->next)

This would relate to CIRCLEQ, not TAILQ.

> 
> I'd prefer something similar:
> 
> #define TAILQ_FOREACH_SAFE(var, head, field, next_var)
>             for (var = (head)->tqh_first, next_var = var->field.tqe_next;
>                   var;
>                   var = next_var, next_var = var->field.tqe_next)
> 
> As it's functionally equivalent and avoids using a GCC-ism ({}).

Won't fly, next_var can become NULL and would be dereferenced without a
prior check. Unless I'm totally blind now, there is no
TAILQ_FOREACH_SAFE without "GCC-ism". But I think I could switch to
CIRCLEQ and add CIRCLEQ_FOREACH_SAFE instead.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Anthony Liguori]

Jan Kiszka wrote:
> Won't fly, next_var can become NULL and would be dereferenced without a
> prior check. Unless I'm totally blind now, there is no
> TAILQ_FOREACH_SAFE without "GCC-ism"

Wouldn't:

(var) ? ({ (next_var) = ((var)->field.tqe_next); 1;}) :0;

Be equivalent to:

(var) ? ((next_var = ((var)->field.tqe_next), var) : var

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Won't fly, next_var can become NULL and would be dereferenced without a
>> prior check. Unless I'm totally blind now, there is no
>> TAILQ_FOREACH_SAFE without "GCC-ism"
> 
> Wouldn't:
> 
> (var) ? ({ (next_var) = ((var)->field.tqe_next); 1;}) :0;
> 
> Be equivalent to:
> 
> (var) ? ((next_var = ((var)->field.tqe_next), var) : var
> 

Yes, indeed. I mixed up that the evaluation order of ,-separated
expressions is not guaranteed, while it is well-defined that the last
operand delivers the result. So let's use this:

---------->

Add TAILQ iterator that allows to safely remove elements while walking
the list.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 sys-queue.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/sys-queue.h b/sys-queue.h
index 3d0773e..e2d3ae7 100644
--- a/sys-queue.h
+++ b/sys-queue.h
@@ -210,6 +210,11 @@ struct {                                                                \
                 (var);                                                  \

                 (var) = ((var)->field.tqe_next))

 

+#define TAILQ_FOREACH_SAFE(var, head, field, next_var)                  \

+        for ((var) = ((head)->tqh_first);                               \

+                (var) ? ((next_var) = ((var)->field.tqe_next), 1) : 0;  \

+                (var) = (next_var))

+

 #define TAILQ_FOREACH_REVERSE(var, head, headname, field)               \

         for ((var) = (*(((struct headname *)((head)->tqh_last))->tqh_last));    \

                 (var);                                                  \

Re: [Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Jamie Lokier]

Anthony Liguori wrote:
> Wouldn't:
> 
> (var) ? ({ (next_var) = ((var)->field.tqe_next); 1;}) :0;
> 
> Be equivalent to:
> 
> (var) ? ((next_var = ((var)->field.tqe_next), var) : var

Simpler:

(var) && ((next_var) = (var)->field.tqe_next, 1)

+ don't forget parens around macro arguments

-- Jamie

[Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Jan Kiszka]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>> Wouldn't:
>>
>> (var) ? ({ (next_var) = ((var)->field.tqe_next); 1;}) :0;
>>
>> Be equivalent to:
>>
>> (var) ? ((next_var = ((var)->field.tqe_next), var) : var
> 
> Simpler:
> 
> (var) && ((next_var) = (var)->field.tqe_next, 1)
> 

Yes, indeed!

Anthony, could you merge this one and its application patch? I need both
of them for my kvm series.

Jan

-------------->

Add TAILQ iterator that allows to safely remove elements while walking
the list.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 sys-queue.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/sys-queue.h b/sys-queue.h
index 3d0773e..ad5c8fb 100644
--- a/sys-queue.h
+++ b/sys-queue.h
@@ -210,6 +210,11 @@ struct {                                                                \
                 (var);                                                  \

                 (var) = ((var)->field.tqe_next))

 

+#define TAILQ_FOREACH_SAFE(var, head, field, next_var)                  \

+        for ((var) = ((head)->tqh_first);                               \

+                (var) && ((next_var) = ((var)->field.tqe_next), 1);     \

+                (var) = (next_var))

+

 #define TAILQ_FOREACH_REVERSE(var, head, headname, field)               \

         for ((var) = (*(((struct headname *)((head)->tqh_last))->tqh_last));    \

                 (var);                                                  \

[Qemu-devel] Re: [PATCH 1/2] Add TAILQ_FOREACH_SAFE

[Anthony Liguori]

Jan Kiszka wrote:
> Jamie Lokier wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> Wouldn't:
>>>
>>> (var) ? ({ (next_var) = ((var)->field.tqe_next); 1;}) :0;
>>>
>>> Be equivalent to:
>>>
>>> (var) ? ((next_var = ((var)->field.tqe_next), var) : var
>>>       
>> Simpler:
>>
>> (var) && ((next_var) = (var)->field.tqe_next, 1)
>>
>>     
>
> Yes, indeed!
>
> Anthony, could you merge this one and its application patch? I need both
> of them for my kvm series.
>   

Applied.  Thanks.

Regards,

Anthony Liguori

> Jan
>
> -------------->
>
> Add TAILQ iterator that allows to safely remove elements while walking
> the list.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
>  sys-queue.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/sys-queue.h b/sys-queue.h
> index 3d0773e..ad5c8fb 100644
> --- a/sys-queue.h
> +++ b/sys-queue.h
> @@ -210,6 +210,11 @@ struct {                                                                \
>                  (var);                                                  \
>
>                  (var) = ((var)->field.tqe_next))
>
>  
>
> +#define TAILQ_FOREACH_SAFE(var, head, field, next_var)                  \
>
> +        for ((var) = ((head)->tqh_first);                               \
>
> +                (var) && ((next_var) = ((var)->field.tqe_next), 1);     \
>
> +                (var) = (next_var))
>
> +
>
>  #define TAILQ_FOREACH_REVERSE(var, head, headname, field)               \
>
>          for ((var) = (*(((struct headname *)((head)->tqh_last))->tqh_last));    \
>
>                  (var);                                                  \
>
>
>   

Re: [Qemu-devel] [PATCH 2/2] Use sys-queue.h for break/watchpoint managment

[Anthony Liguori]

Jan Kiszka wrote:
> This switches cpu_break/watchpoint_* to TAILQ wrappers, simplifying the
> code and also fixing a use after release issue in
> cpu_break/watchpoint_remove_all.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

Applied.  Thanks.

Regards,

Anthony Liguori