| Message ID | 1270822934-8623-3-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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; };
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- block.c | 44 +++++++++++++++++++++++--------------------- block_int.h | 3 ++- 2 files changed, 25 insertions(+), 22 deletions(-)