Re: [Qemu-devel] [PATCH 07/23] block: Eliminate bdrv_iterate(), use bdrv_next()

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 10 Sep 2014 à 10:13:36 (+0200), Markus Armbruster wrote :
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block-migration.c     | 30 +++++++++++++++---------------
>  block.c               |  9 ---------
>  blockdev.c            | 31 +++++++++++++------------------
>  include/block/block.h |  2 --
>  monitor.c             | 33 +++++++++------------------------
>  5 files changed, 37 insertions(+), 68 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 3ad31a2..cb3e16c 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -343,12 +343,25 @@ static void unset_dirty_tracking(void)
>      }
>  }
>  
> -static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
> +static void init_blk_migration(QEMUFile *f)
>  {
> +    BlockDriverState *bs;
>      BlkMigDevState *bmds;
>      int64_t sectors;
>  
> -    if (!bdrv_is_read_only(bs)) {
> +    block_mig_state.submitted = 0;
> +    block_mig_state.read_done = 0;
> +    block_mig_state.transferred = 0;
> +    block_mig_state.total_sector_sum = 0;
> +    block_mig_state.prev_progress = -1;
> +    block_mig_state.bulk_completed = 0;
> +    block_mig_state.zero_blocks = migrate_zero_blocks();
> +
> +    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
> +        if (bdrv_is_read_only(bs)) {
> +            continue;
> +        }
> +
>          sectors = bdrv_nb_sectors(bs);
>          if (sectors <= 0) {
>              return;
> @@ -378,19 +391,6 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>      }
>  }
>  
> -static void init_blk_migration(QEMUFile *f)
> -{
> -    block_mig_state.submitted = 0;
> -    block_mig_state.read_done = 0;
> -    block_mig_state.transferred = 0;
> -    block_mig_state.total_sector_sum = 0;
> -    block_mig_state.prev_progress = -1;
> -    block_mig_state.bulk_completed = 0;
> -    block_mig_state.zero_blocks = migrate_zero_blocks();
> -
> -    bdrv_iterate(init_blk_migration_it, NULL);
> -}
> -
>  /* Called with no lock taken.  */
>  
>  static int blk_mig_save_bulked_block(QEMUFile *f)
> diff --git a/block.c b/block.c
> index 89f9cf0..593d89b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3889,15 +3889,6 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
>      return blk ? blk_bs(blk) : NULL;
>  }
>  
> -void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> -{
> -    BlockDriverState *bs;
> -
> -    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
> -        it(opaque, bs);
> -    }
> -}
> -
>  const char *bdrv_get_device_name(BlockDriverState *bs)
>  {
>      return bs->device_name;
> diff --git a/blockdev.c b/blockdev.c
> index 791f6d9..353563e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2520,26 +2520,21 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> -static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> -{
> -    BlockJobInfoList **prev = opaque;
> -    BlockJob *job = bs->job;
> -
> -    if (job) {
> -        BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
> -        elem->value = block_job_query(bs->job);
> -        (*prev)->next = elem;
> -        *prev = elem;
> -    }
> -}
> -
>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
> -    /* Dummy is a fake list element for holding the head pointer */
> -    BlockJobInfoList dummy = {};
> -    BlockJobInfoList *prev = &dummy;
> -    bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
> -    return dummy.next;
> +    BlockJobInfoList *head = NULL, **p_next = &head;
> +    BlockDriverState *bs;
> +
> +    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
> +        if (bs->job) {
> +            BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
> +            elem->value = block_job_query(bs->job);
> +            *p_next = elem;
> +            p_next = &elem->next;
> +        }
> +    }
> +
> +    return head;
>  }
>  
>  QemuOptsList qemu_common_drive_opts = {
> diff --git a/include/block/block.h b/include/block/block.h
> index 95139c0..8cf9ea3 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -409,8 +409,6 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
>                                   Error **errp);
>  bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
>  BlockDriverState *bdrv_next(BlockDriverState *bs);
> -void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> -                  void *opaque);
>  int bdrv_is_encrypted(BlockDriverState *bs);
>  int bdrv_key_required(BlockDriverState *bs);
>  int bdrv_set_key(BlockDriverState *bs, const char *key);
> diff --git a/monitor.c b/monitor.c
> index 34cee74..4ae66df 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4208,24 +4208,6 @@ static void file_completion(Monitor *mon, const char *input)
>      closedir(ffs);
>  }
>  
> -typedef struct MonitorBlockComplete {
> -    Monitor *mon;
> -    const char *input;
> -} MonitorBlockComplete;
> -
> -static void block_completion_it(void *opaque, BlockDriverState *bs)
> -{
> -    const char *name = bdrv_get_device_name(bs);
> -    MonitorBlockComplete *mbc = opaque;
> -    Monitor *mon = mbc->mon;
> -    const char *input = mbc->input;
> -
> -    if (input[0] == '\0' ||
> -        !strncmp(name, (char *)input, strlen(input))) {
> -        readline_add_completion(mon->rs, name);
> -    }
> -}
> -
>  static const char *next_arg_type(const char *typestr)
>  {
>      const char *p = strchr(typestr, ':');
> @@ -4663,9 +4645,9 @@ static void monitor_find_completion_by_table(Monitor *mon,
>  {
>      const char *cmdname;
>      int i;
> -    const char *ptype, *str;
> +    const char *ptype, *str, *name;
>      const mon_cmd_t *cmd;
> -    MonitorBlockComplete mbs;
> +    BlockDriverState *bs;
>  
>      if (nb_args <= 1) {
>          /* command completion */
> @@ -4717,10 +4699,13 @@ static void monitor_find_completion_by_table(Monitor *mon,
>              break;
>          case 'B':
>              /* block device name completion */
> -            mbs.mon = mon;
> -            mbs.input = str;

> -            readline_set_completion_index(mon->rs, strlen(str));

Why is this line removed ?
In monitor readline_set_completion_index seems to work in pair with readline_add_completion.
Either this line should be removed and readline_add_completion too or the oposite.

> -            bdrv_iterate(block_completion_it, &mbs);
> +            for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
> +                name = bdrv_get_device_name(bs);
> +                if (str[0] == '\0' ||
> +                    !strncmp(name, str, strlen(str))) {
> +                    readline_add_completion(mon->rs, name);
> +                }
> +            }
>              break;
>          case 's':
>          case 'S':
> -- 
> 1.9.3
> 
>