Re: [PATCH v5 3/6] blkdebug: track all actions

[Max Reitz]

On 14.06.21 10:29, Emanuele Giuseppe Esposito wrote:

Add a counter for each action that a rule can trigger.
This is mainly used to keep track of how many coroutine_yield()
we need to perform after processing all rules in the list.

Co-developed-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
  block/blkdebug.c | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index e8fdf7b056..6bdeb2c7b3 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -74,6 +74,7 @@ enum {
  ACTION_INJECT_ERROR,
  ACTION_SET_STATE,
  ACTION_SUSPEND,
+ACTION__MAX,
  };
  
  typedef struct BlkdebugRule {

@@ -791,22 +792,22 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
  qemu_coroutine_yield();
  }
  
-static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,

-bool injected)
+static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+ int *action_count)


I would have liked a comment above this function explaining that 
`action_count` is not merely an int pointer, but actually an 
int[ACTION__MAX] pointer.


But it’s too late to complain about that now. O:)


  {
  BDRVBlkdebugState *s = bs->opaque;
  
  /* Only process rules for the current state */

  if (rule->state && rule->state != s->state) {
-return injected;
+return;
  }
  
  /* Take the action */

+action_count[rule->action]++;
  switch (rule->action) {
  case ACTION_INJECT_ERROR:
-if (!injected) {
+if (action_count[ACTION_INJECT_ERROR] == 1) {
  QSIMPLEQ_INIT(>active_rules);


(I don’t quite understand this part – why do we clear the list of active 
rules here?  And why only if a new error is injected?  For example, if I 
have an inject-error rule that should only fire on state 1, and then the 
state changes to state 2, it stays active until a new error is injected, 
which doesn’t make sense to me.  But that has nothing to do with this 
series, of course.  I’m just wondering.)


Max


-injected = true;
  }
  QSIMPLEQ_INSERT_HEAD(>active_rules, rule, active_next);
  break;

   

[PATCH v5 3/6] blkdebug: track all actions

[Emanuele Giuseppe Esposito]

Add a counter for each action that a rule can trigger.
This is mainly used to keep track of how many coroutine_yield()
we need to perform after processing all rules in the list.

Co-developed-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/blkdebug.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index e8fdf7b056..6bdeb2c7b3 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -74,6 +74,7 @@ enum {
 ACTION_INJECT_ERROR,
 ACTION_SET_STATE,
 ACTION_SUSPEND,
+ACTION__MAX,
 };
 
 typedef struct BlkdebugRule {
@@ -791,22 +792,22 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
 qemu_coroutine_yield();
 }
 
-static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
-bool injected)
+static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+ int *action_count)
 {
 BDRVBlkdebugState *s = bs->opaque;
 
 /* Only process rules for the current state */
 if (rule->state && rule->state != s->state) {
-return injected;
+return;
 }
 
 /* Take the action */
+action_count[rule->action]++;
 switch (rule->action) {
 case ACTION_INJECT_ERROR:
-if (!injected) {
+if (action_count[ACTION_INJECT_ERROR] == 1) {
 QSIMPLEQ_INIT(>active_rules);
-injected = true;
 }
 QSIMPLEQ_INSERT_HEAD(>active_rules, rule, active_next);
 break;
@@ -819,21 +820,19 @@ static bool process_rule(BlockDriverState *bs, struct 
BlkdebugRule *rule,
 suspend_request(bs, rule);
 break;
 }
-return injected;
 }
 
 static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
 BDRVBlkdebugState *s = bs->opaque;
 struct BlkdebugRule *rule, *next;
-bool injected;
+int actions_count[ACTION__MAX] = { 0 };
 
 assert((int)event >= 0 && event < BLKDBG__MAX);
 
-injected = false;
 s->new_state = s->state;
 QLIST_FOREACH_SAFE(rule, >rules[event], next, next) {
-injected = process_rule(bs, rule, injected);
+process_rule(bs, rule, actions_count);
 }
 s->state = s->new_state;
 }
-- 
2.31.1