[Qemu-devel] [PATCH 0/2] block: Cleanups

[Qemu-devel] [PATCH 0/2] block: Cleanups

[Stefan Hajnoczi]

Small cleanups to block.c and friends.

These patches apply to qemu.git and kevin's block branch.

[Qemu-devel] [PATCH 1/2] block: Do not export bdrv_first

[Stefan Hajnoczi]

The bdrv_first linked list of BlockDriverStates is currently extern so
that block migration can iterate the list.  However, since there is
already a bdrv_iterate() function there is no need to expose bdrv_first.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block-migration.c |   63 +++++++++++++++++++++++++++-------------------------
 block.c           |    2 +-
 block_int.h       |    2 -
 3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 92349a2..7d04d6d 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -230,12 +230,42 @@ static void set_dirty_tracking(int enable)
     }
 }
 
-static void init_blk_migration(Monitor *mon, QEMUFile *f)
+static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
 {
+    Monitor *mon = opaque;
     BlkMigDevState *bmds;
-    BlockDriverState *bs;
     int64_t sectors;
 
+    if (bs->type == BDRV_TYPE_HD) {
+        sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+        if (sectors == 0) {
+            return;
+        }
+
+        bmds = qemu_mallocz(sizeof(BlkMigDevState));
+        bmds->bs = bs;
+        bmds->bulk_completed = 0;
+        bmds->total_sectors = sectors;
+        bmds->completed_sectors = 0;
+        bmds->shared_base = block_mig_state.shared_base;
+
+        block_mig_state.total_sector_sum += sectors;
+
+        if (bmds->shared_base) {
+            monitor_printf(mon, "Start migration for %s with shared base "
+                                "image\n",
+                           bs->device_name);
+        } else {
+            monitor_printf(mon, "Start full migration for %s\n",
+                           bs->device_name);
+        }
+
+        QSIMPLEQ_INSERT_TAIL(&block_mig_state.bmds_list, bmds, entry);
+    }
+}
+
+static void init_blk_migration(Monitor *mon, QEMUFile *f)
+{
     block_mig_state.submitted = 0;
     block_mig_state.read_done = 0;
     block_mig_state.transferred = 0;
@@ -245,34 +275,7 @@ static void init_blk_migration(Monitor *mon, QEMUFile *f)
     block_mig_state.total_time = 0;
     block_mig_state.reads = 0;
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next) {
-        if (bs->type == BDRV_TYPE_HD) {
-            sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
-            if (sectors == 0) {
-                continue;
-            }
-
-            bmds = qemu_mallocz(sizeof(BlkMigDevState));
-            bmds->bs = bs;
-            bmds->bulk_completed = 0;
-            bmds->total_sectors = sectors;
-            bmds->completed_sectors = 0;
-            bmds->shared_base = block_mig_state.shared_base;
-
-            block_mig_state.total_sector_sum += sectors;
-
-            if (bmds->shared_base) {
-                monitor_printf(mon, "Start migration for %s with shared base "
-                                    "image\n",
-                               bs->device_name);
-            } else {
-                monitor_printf(mon, "Start full migration for %s\n",
-                               bs->device_name);
-            }
-
-            QSIMPLEQ_INSERT_TAIL(&block_mig_state.bmds_list, bmds, entry);
-        }
-    }
+    bdrv_iterate(init_blk_migration_it, mon);
 }
 
 static int blk_mig_save_bulked_block(Monitor *mon, QEMUFile *f)
diff --git a/block.c b/block.c
index ed4c819..61da183 100644
--- a/block.c
+++ b/block.c
@@ -55,7 +55,7 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
 static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
                          const uint8_t *buf, int nb_sectors);
 
-BlockDriverState *bdrv_first;
+static BlockDriverState *bdrv_first;
 
 static BlockDriver *first_drv;
 
diff --git a/block_int.h b/block_int.h
index e7e1e7e..d5a808d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -200,8 +200,6 @@ void qemu_aio_release(void *p);
 
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
 
-extern BlockDriverState *bdrv_first;
-
 #ifdef _WIN32
 int is_windows_drive(const char *filename);
 #endif
-- 
1.7.0

Re: [Qemu-devel] [PATCH 1/2] block: Do not export bdrv_first

[Kevin Wolf]

Am 09.04.2010 16:22, schrieb Stefan Hajnoczi:
> The bdrv_first linked list of BlockDriverStates is currently extern so
> that block migration can iterate the list.  However, since there is
> already a bdrv_iterate() function there is no need to expose bdrv_first.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Looks good. Applied to the block branch.

Kevin

[Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c     |   44 +++++++++++++++++++++++---------------------
 block_int.h |    3 ++-
 2 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 61da183..86d2504 100644
--- a/block.c
+++ b/block.c
@@ -55,7 +55,8 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
 static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
                          const uint8_t *buf, int nb_sectors);
 
-static BlockDriverState *bdrv_first;
+static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
+    QTAILQ_HEAD_INITIALIZER(bdrv_states);
 
 static BlockDriver *first_drv;
 
@@ -148,16 +149,12 @@ void bdrv_register(BlockDriver *bdrv)
 /* create a new block device (by default it is empty) */
 BlockDriverState *bdrv_new(const char *device_name)
 {
-    BlockDriverState **pbs, *bs;
+    BlockDriverState *bs;
 
     bs = qemu_mallocz(sizeof(BlockDriverState));
     pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
     if (device_name[0] != '\0') {
-        /* insert at the end */
-        pbs = &bdrv_first;
-        while (*pbs != NULL)
-            pbs = &(*pbs)->next;
-        *pbs = bs;
+        QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
     return bs;
 }
@@ -545,13 +542,15 @@ void bdrv_close(BlockDriverState *bs)
 
 void bdrv_delete(BlockDriverState *bs)
 {
-    BlockDriverState **pbs;
+    BlockDriverState *bs1;
 
-    pbs = &bdrv_first;
-    while (*pbs != bs && *pbs != NULL)
-        pbs = &(*pbs)->next;
-    if (*pbs == bs)
-        *pbs = bs->next;
+    /* remove from list, if necessary */
+    QTAILQ_FOREACH(bs1, &bdrv_states, list) {
+        if (bs1 == bs) {
+            QTAILQ_REMOVE(&bdrv_states, bs, list);
+            break;
+        }
+    }
 
     bdrv_close(bs);
     qemu_free(bs);
@@ -1172,9 +1171,10 @@ BlockDriverState *bdrv_find(const char *name)
 {
     BlockDriverState *bs;
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next) {
-        if (!strcmp(name, bs->device_name))
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        if (!strcmp(name, bs->device_name)) {
             return bs;
+        }
     }
     return NULL;
 }
@@ -1183,7 +1183,7 @@ void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
 {
     BlockDriverState *bs;
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next) {
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
         it(opaque, bs);
     }
 }
@@ -1203,10 +1203,12 @@ void bdrv_flush_all(void)
 {
     BlockDriverState *bs;
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next)
-        if (bs->drv && !bdrv_is_read_only(bs) && 
-            (!bdrv_is_removable(bs) || bdrv_is_inserted(bs)))
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        if (bs->drv && !bdrv_is_read_only(bs) &&
+            (!bdrv_is_removable(bs) || bdrv_is_inserted(bs))) {
             bdrv_flush(bs);
+        }
+    }
 }
 
 /*
@@ -1340,7 +1342,7 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
     bs_list = qlist_new();
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next) {
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
         QObject *bs_obj;
         const char *type = "unknown";
 
@@ -1445,7 +1447,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
 
     devices = qlist_new();
 
-    for (bs = bdrv_first; bs != NULL; bs = bs->next) {
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
         obj = qobject_from_jsonf("{ 'device': %s, 'stats': {"
                                  "'rd_bytes': %" PRId64 ","
                                  "'wr_bytes': %" PRId64 ","
diff --git a/block_int.h b/block_int.h
index d5a808d..466a38c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -26,6 +26,7 @@
 
 #include "block.h"
 #include "qemu-option.h"
+#include "qemu-queue.h"
 
 #define BLOCK_FLAG_ENCRYPT	1
 #define BLOCK_FLAG_COMPRESS	2
@@ -180,7 +181,7 @@ struct BlockDriverState {
     char device_name[32];
     unsigned long *dirty_bitmap;
     int64_t dirty_count;
-    BlockDriverState *next;
+    QTAILQ_ENTRY(BlockDriverState) list;
     void *private;
 };
 
-- 
1.7.0

Re: [Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Kevin Wolf]

Am 09.04.2010 16:22, schrieb Stefan Hajnoczi:
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c     |   44 +++++++++++++++++++++++---------------------
>  block_int.h |    3 ++-
>  2 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 61da183..86d2504 100644
> --- a/block.c
> +++ b/block.c
> @@ -55,7 +55,8 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
>  static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
>                           const uint8_t *buf, int nb_sectors);
>  
> -static BlockDriverState *bdrv_first;
> +static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> +    QTAILQ_HEAD_INITIALIZER(bdrv_states);
>  
>  static BlockDriver *first_drv;
>  
> @@ -148,16 +149,12 @@ void bdrv_register(BlockDriver *bdrv)
>  /* create a new block device (by default it is empty) */
>  BlockDriverState *bdrv_new(const char *device_name)
>  {
> -    BlockDriverState **pbs, *bs;
> +    BlockDriverState *bs;
>  
>      bs = qemu_mallocz(sizeof(BlockDriverState));
>      pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
>      if (device_name[0] != '\0') {
> -        /* insert at the end */
> -        pbs = &bdrv_first;
> -        while (*pbs != NULL)
> -            pbs = &(*pbs)->next;
> -        *pbs = bs;
> +        QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>      }
>      return bs;
>  }
> @@ -545,13 +542,15 @@ void bdrv_close(BlockDriverState *bs)
>  
>  void bdrv_delete(BlockDriverState *bs)
>  {
> -    BlockDriverState **pbs;
> +    BlockDriverState *bs1;
>  
> -    pbs = &bdrv_first;
> -    while (*pbs != bs && *pbs != NULL)
> -        pbs = &(*pbs)->next;
> -    if (*pbs == bs)
> -        *pbs = bs->next;
> +    /* remove from list, if necessary */
> +    QTAILQ_FOREACH(bs1, &bdrv_states, list) {
> +        if (bs1 == bs) {
> +            QTAILQ_REMOVE(&bdrv_states, bs, list);
> +            break;
> +        }
> +    }

This loop looks strange, what is it used for? We had only a next pointer
previously, so we needed to search the element. A QTAILQ has both prev
and next pointers though, so you should be able to directly use
QTAILQ_REMOVE.

The rest of it looks good.

Kevin

Re: [Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Stefan Hajnoczi]

On Fri, Apr 9, 2010 at 5:47 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> @@ -545,13 +542,15 @@ void bdrv_close(BlockDriverState *bs)
>>
>>  void bdrv_delete(BlockDriverState *bs)
>>  {
>> -    BlockDriverState **pbs;
>> +    BlockDriverState *bs1;
>>
>> -    pbs = &bdrv_first;
>> -    while (*pbs != bs && *pbs != NULL)
>> -        pbs = &(*pbs)->next;
>> -    if (*pbs == bs)
>> -        *pbs = bs->next;
>> +    /* remove from list, if necessary */
>> +    QTAILQ_FOREACH(bs1, &bdrv_states, list) {
>> +        if (bs1 == bs) {
>> +            QTAILQ_REMOVE(&bdrv_states, bs, list);
>> +            break;
>> +        }
>> +    }
>
> This loop looks strange, what is it used for? We had only a next pointer
> previously, so we needed to search the element. A QTAILQ has both prev
> and next pointers though, so you should be able to directly use
> QTAILQ_REMOVE.

Only named BlockDriverStates are added to the tail queue.  Those with
an empty string as their name will not be on the tail queue.

The QTAILQ_REMOVE macro will not work if the element isn't on the tail
queue.  I didn't see a clean way to check if an element is on a tail
queue using qemu-queue.h, so I kept the search behavior.  The check I
want is tge_prev != NULL, I think.

I see three options:
1. Leave the search.
2. Modify qemu-queue.h to add a QTAILQ_ON_LIST(elm) macro.
3. Break the QTAILQ abstraction and test tge_prev directly.

What do you think?

Stefan

Re: [Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Stefan Hajnoczi]

On Fri, Apr 9, 2010 at 7:17 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> I see three options:
> 1. Leave the search.
> 2. Modify qemu-queue.h to add a QTAILQ_ON_LIST(elm) macro.
> 3. Break the QTAILQ abstraction and test tge_prev directly.

I see a nicer option: check device_name[0].  Patch v2 sent.

Stefan

Re: [Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Kevin Wolf]

Am 10.04.2010 08:04, schrieb Stefan Hajnoczi:
> On Fri, Apr 9, 2010 at 7:17 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> I see three options:
>> 1. Leave the search.
>> 2. Modify qemu-queue.h to add a QTAILQ_ON_LIST(elm) macro.
>> 3. Break the QTAILQ abstraction and test tge_prev directly.
> 
> I see a nicer option: check device_name[0].  Patch v2 sent.

Yup, this is exactly what I was just about to suggest.

Of course, QTAILQ_ON_LIST would be nice to have either, but I'm afraid
it's not possible without putting the loop into that macro.

Kevin