Re: [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead

2014-09-11 Thread Benoît Canet 
The Wednesday 10 Sep 2014 à 10:13:35 (+0200), Markus Armbruster wrote :
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  block.c   | 43 +++
  block/block-backend.c |  4 
  include/block/block_int.h |  2 --
  3 files changed, 23 insertions(+), 26 deletions(-)
 
 diff --git a/block.c b/block.c
 index a6c03da..89f9cf0 100644
 --- a/block.c
 +++ b/block.c
 @@ -24,6 +24,7 @@
  #include config-host.h
  #include qemu-common.h
  #include trace.h
 +#include sysemu/block-backend.h
  #include block/block_int.h
  #include block/blockjob.h
  #include qemu/module.h
 @@ -90,9 +91,6 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque);
  static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
  int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
  
 -static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
 -QTAILQ_HEAD_INITIALIZER(bdrv_states);
 -
  static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
  QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
  
 @@ -355,9 +353,6 @@ BlockDriverState *bdrv_new_named(const char *device_name, 
 Error **errp)
  bs = bdrv_new();
  
  pstrcpy(bs-device_name, sizeof(bs-device_name), device_name);
 -if (device_name[0] != '\0') {
 -QTAILQ_INSERT_TAIL(bdrv_states, bs, device_list);
 -}
  
  return bs;
  }
 @@ -1888,7 +1883,7 @@ void bdrv_close_all(void)
  {
  BlockDriverState *bs;
  
 -QTAILQ_FOREACH(bs, bdrv_states, device_list) {
 +for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
  AioContext *aio_context = bdrv_get_aio_context(bs);
  
  aio_context_acquire(aio_context);
 @@ -1939,7 +1934,7 @@ void bdrv_drain_all(void)
  while (busy) {
  busy = false;
  
 -QTAILQ_FOREACH(bs, bdrv_states, device_list) {
 +for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
  AioContext *aio_context = bdrv_get_aio_context(bs);
  bool bs_busy;
  
 @@ -1960,9 +1955,6 @@ void bdrv_drain_all(void)
 Also, NULL terminate the device_name to prevent double remove */
  void bdrv_make_anon(BlockDriverState *bs)
  {
 -if (bs-device_name[0] != '\0') {
 -QTAILQ_REMOVE(bdrv_states, bs, device_list);
 -}
  bs-device_name[0] = '\0';
  if (bs-node_name[0] != '\0') {
  QTAILQ_REMOVE(graph_bdrv_states, bs, node_list);
 @@ -2016,10 +2008,9 @@ static void bdrv_move_feature_fields(BlockDriverState 
 *bs_dest,
  /* job */
  bs_dest-job= bs_src-job;
  
 -/* keep the same entry in bdrv_states */
  pstrcpy(bs_dest-device_name, sizeof(bs_dest-device_name),
  bs_src-device_name);
 -bs_dest-device_list = bs_src-device_list;
 +
  memcpy(bs_dest-op_blockers, bs_src-op_blockers,
 sizeof(bs_dest-op_blockers));
  }
 @@ -2363,7 +2354,7 @@ int bdrv_commit_all(void)
  {
  BlockDriverState *bs;
  
 -QTAILQ_FOREACH(bs, bdrv_states, device_list) {
 +for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
  AioContext *aio_context = bdrv_get_aio_context(bs);
  
  aio_context_acquire(aio_context);
 @@ -3807,7 +3798,7 @@ BlockDriverState *bdrv_find(const char *name)
  {
  BlockDriverState *bs;
  
 -QTAILQ_FOREACH(bs, bdrv_states, device_list) {
 +for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
  if (!strcmp(name, bs-device_name)) {
  return bs;
  }
 @@ -3888,17 +3879,21 @@ bool bdrv_chain_contains(BlockDriverState *top, 
 BlockDriverState *base)
  
  BlockDriverState *bdrv_next(BlockDriverState *bs)
  {
 -if (!bs) {
 -return QTAILQ_FIRST(bdrv_states);
 -}
 -return QTAILQ_NEXT(bs, device_list);

 +BlockBackend *blk;
 +
 +for (blk = blk_next(bs ? bs-blk : NULL);
 + blk  !blk_bs(blk);
 + blk = blk_next(blk))
 +;
 +
 +return blk ? blk_bs(blk) : NULL;

I find ?: hard to read and I know at least one maintainer
prefers simples ifs to it.

BlockDriverState *bdrv_next(BlockDriverState *bs)
{
BlockBackend *blk = NULL;

/* if a BDS is provided use it's block backend as a starting point */
if (bs) {
blk = bl-blk;
}

/* looking for the next block backend having a BDS attached */
for (blk = blk_next(blk);
 blk  !blk_bs(blk);
 blk = blk_next(blk))
 ;

   /* the search was successfull */
   if (blk) {
   return blk_bs(blk);
   }

   return NULL;
}

I think getting rid of ?: spread the brain load sequentially by being less 
compact
but anyway your code is correct so it's up to you.

  }
  
  void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void 
 *opaque)
  {
  BlockDriverState *bs;
  
 -QTAILQ_FOREACH(bs, bdrv_states, device_list) {
 +for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
  it(opaque, bs);
  }
  }
 @@ -3918,7 +3913,7 @@ int bdrv_flush_all(void)
  BlockDriverState *bs;
  int result = 0;
  
 -

[Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead

2014-09-10 Thread Markus Armbruster 
Signed-off-by: Markus Armbruster arm...@redhat.com
---
 block.c   | 43 +++
 block/block-backend.c |  4 
 include/block/block_int.h |  2 --
 3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index a6c03da..89f9cf0 100644
--- a/block.c
+++ b/block.c
@@ -24,6 +24,7 @@
 #include config-host.h
 #include qemu-common.h
 #include trace.h
+#include sysemu/block-backend.h
 #include block/block_int.h
 #include block/blockjob.h
 #include qemu/module.h
@@ -90,9 +91,6 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
 
-static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
-QTAILQ_HEAD_INITIALIZER(bdrv_states);
-
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
 QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
@@ -355,9 +353,6 @@ BlockDriverState *bdrv_new_named(const char *device_name, 
Error **errp)
 bs = bdrv_new();
 
 pstrcpy(bs-device_name, sizeof(bs-device_name), device_name);
-if (device_name[0] != '\0') {
-QTAILQ_INSERT_TAIL(bdrv_states, bs, device_list);
-}
 
 return bs;
 }
@@ -1888,7 +1883,7 @@ void bdrv_close_all(void)
 {
 BlockDriverState *bs;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 
 aio_context_acquire(aio_context);
@@ -1939,7 +1934,7 @@ void bdrv_drain_all(void)
 while (busy) {
 busy = false;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 bool bs_busy;
 
@@ -1960,9 +1955,6 @@ void bdrv_drain_all(void)
Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
 {
-if (bs-device_name[0] != '\0') {
-QTAILQ_REMOVE(bdrv_states, bs, device_list);
-}
 bs-device_name[0] = '\0';
 if (bs-node_name[0] != '\0') {
 QTAILQ_REMOVE(graph_bdrv_states, bs, node_list);
@@ -2016,10 +2008,9 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
 /* job */
 bs_dest-job= bs_src-job;
 
-/* keep the same entry in bdrv_states */
 pstrcpy(bs_dest-device_name, sizeof(bs_dest-device_name),
 bs_src-device_name);
-bs_dest-device_list = bs_src-device_list;
+
 memcpy(bs_dest-op_blockers, bs_src-op_blockers,
sizeof(bs_dest-op_blockers));
 }
@@ -2363,7 +2354,7 @@ int bdrv_commit_all(void)
 {
 BlockDriverState *bs;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 
 aio_context_acquire(aio_context);
@@ -3807,7 +3798,7 @@ BlockDriverState *bdrv_find(const char *name)
 {
 BlockDriverState *bs;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 if (!strcmp(name, bs-device_name)) {
 return bs;
 }
@@ -3888,17 +3879,21 @@ bool bdrv_chain_contains(BlockDriverState *top, 
BlockDriverState *base)
 
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
-if (!bs) {
-return QTAILQ_FIRST(bdrv_states);
-}
-return QTAILQ_NEXT(bs, device_list);
+BlockBackend *blk;
+
+for (blk = blk_next(bs ? bs-blk : NULL);
+ blk  !blk_bs(blk);
+ blk = blk_next(blk))
+;
+
+return blk ? blk_bs(blk) : NULL;
 }
 
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
 {
 BlockDriverState *bs;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 it(opaque, bs);
 }
 }
@@ -3918,7 +3913,7 @@ int bdrv_flush_all(void)
 BlockDriverState *bs;
 int result = 0;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 int ret;
 
@@ -5065,7 +5060,7 @@ void bdrv_invalidate_cache_all(Error **errp)
 BlockDriverState *bs;
 Error *local_err = NULL;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 
 aio_context_acquire(aio_context);
@@ -5082,7 +5077,7 @@ void bdrv_clear_incoming_migration_all(void)
 {
 BlockDriverState *bs;
 
-QTAILQ_FOREACH(bs, bdrv_states, device_list) {
+for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 
 aio_context_acquire(aio_context);
@@ -5918,7 +5913,7 @@ bool