[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_n
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead |
Date: |
Thu, 11 Sep 2014 12:25:12 +0200 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
The Wednesday 10 Sep 2014 à 10:13:35 (+0200), Markus Armbruster wrote :
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
> 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;
>
> - 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 bdrv_is_first_non_filter(BlockDriverState
> *candidate)
> BlockDriverState *bs;
>
> /* walk down the bs forest recursively */
> - QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> + for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
> bool perm;
>
> /* try to recurse in this top level bs */
> diff --git a/block/block-backend.c b/block/block-backend.c
> index ae51f7f..c0876fa 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -191,6 +191,10 @@ BlockDriverState *blk_detach_bs(BlockBackend *blk)
> {
> BlockDriverState *bs = blk->bs;
>
> + /*
> + * Must bdrv_unref() before zapping bs->blk, or else
> + * bdrv_drain_all() will miss bs.
> + */
> if (bs) {
> bs->blk = NULL;
> blk->bs = NULL;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a04c852..d5de08b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -396,8 +396,6 @@ struct BlockDriverState {
> QTAILQ_ENTRY(BlockDriverState) node_list;
> /* Device name is the name associated with the "drive" the guest sees */
> char device_name[32];
> - /* element of the list of "drives" the guest sees */
> - QTAILQ_ENTRY(BlockDriverState) device_list;
> QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
> int refcnt;
>
> --
> 1.9.3
>
- [Qemu-devel] [PATCH 00/23] Split BlockBackend off BDS with an axe, Markus Armbruster, 2014/09/10
- [Qemu-devel] [PATCH 03/23] block: Connect BlockBackend to BlockDriverState, Markus Armbruster, 2014/09/10
- [Qemu-devel] [PATCH 09/23] block: Merge BlockBackend and BlockDriverState name spaces, Markus Armbruster, 2014/09/10
- [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead, Markus Armbruster, 2014/09/10
- Re: [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead,
Benoît Canet <=
- [Qemu-devel] [PATCH 07/23] block: Eliminate bdrv_iterate(), use bdrv_next(), Markus Armbruster, 2014/09/10
- [Qemu-devel] [PATCH 01/23] block: Split bdrv_new_named() off bdrv_new(), Markus Armbruster, 2014/09/10
- Re: [Qemu-devel] [PATCH 01/23] block: Split bdrv_new_named() off bdrv_new(), Eric Blake, 2014/09/10
- Re: [Qemu-devel] [PATCH 01/23] block: Split bdrv_new_named() off bdrv_new(), Benoît Canet, 2014/09/10