Patchwork [2/2] block: Convert bdrv_first to QTAILQ

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 9, 2010, 2:22 p.m.
Message ID <1270822934-8623-3-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/49851/
State New
Headers show

Comments

Stefan Hajnoczi - April 9, 2010, 2:22 p.m.
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c     |   44 +++++++++++++++++++++++---------------------
 block_int.h |    3 ++-
 2 files changed, 25 insertions(+), 22 deletions(-)
Kevin Wolf - April 9, 2010, 4:47 p.m.
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
Stefan Hajnoczi - April 9, 2010, 6:17 p.m.
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
Stefan Hajnoczi - April 10, 2010, 6:04 a.m.
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
Kevin Wolf - April 10, 2010, 7:56 a.m.
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

Patch

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;
 };