Re: [Qemu-devel] [PATCH v2 02/23] block: New BlockBackend

["Benoît Canet" <benoit.canet@nodalink.com>]

> --- a/block.c
> +++ b/block.c
> @@ -2119,10 +2119,11 @@ static void bdrv_delete(BlockDriverState *bs)
>  
>      bdrv_close(bs);
>  


> +    drive_info_del(drive_get_by_blockdev(bs));
> +
>      /* remove from list, if necessary */
>      bdrv_make_anon(bs);
>  
> -    drive_info_del(drive_get_by_blockdev(bs));

Do we really want this move ?
Or is it an artefact of your work on the preparation series ?

>      g_free(bs);
>  }
>  

> + * Return the BlockBackend with name @name if it exists, else null.

> + * @name must not be null.
The contract is that no one will call blk_by_name(NULL);

> + */
> +BlockBackend *blk_by_name(const char *name)
> +{
> +    BlockBackend *blk;

Can we ?

     assert(name);

> +
> +    QTAILQ_FOREACH(blk, &blk_backends, link) {
> +        if (!strcmp(name, blk->name)) {
> +            return blk;
> +        }
> +    }
> +    return NULL;
> +}


> +    /*
> +     * Hairy special case: if do_drive_del() has made dinfo->bdrv
> +     * anonymous, it also unref'ed the associated BlockBackend.
> +     */

Then you are filling the other case here.

maybe

    /*
     * Hairy special case: if do_drive_del() has made dinfo->bdrv
     * anonymous, it also unref'ed the associated BlockBackend.
     * Process the other case here.
     */

would further explain you intents.


> +    if (dinfo->bdrv->device_name[0]) {
> +        blk_unref(blk_by_name(dinfo->bdrv->device_name));
> +    }
> +
>      g_free(dinfo->id);
>      QTAILQ_REMOVE(&drives, dinfo, next);
>      g_free(dinfo->serial);
> @@ -307,6 +317,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      int ro = 0;
>      int bdrv_flags = 0;
>      int on_read_error, on_write_error;
> +    BlockBackend *blk;
>      BlockDriverState *bs;
>      DriveInfo *dinfo;
>      ThrottleConfig cfg;
> @@ -463,9 +474,13 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      }

>      const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
> +    BlockBackend *blk1, *blk2;
>      BlockDriverState *bs1, *bs2;
>      int64_t total_sectors1, total_sectors2;
>      uint8_t *buf1 = NULL, *buf2 = NULL;
> @@ -1011,18 +1022,20 @@ static int img_compare(int argc, char **argv)
>          goto out3;
>      }
>  
> +    blk1 = blk_new("image 1", &error_abort);
>      bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
>      if (!bs1) {
>          error_report("Can't open file %s", filename1);
>          ret = 2;
> -        goto out3;
> +        goto out2;

Here we allocate blk1 and bs1 an if bs1 is null (error) we jump to out2 which is:

>  out2:
>      bdrv_unref(bs1);
> +    blk_unref(blk1);

It's a bit strange that we will bdrv_unref(NULL) in this case.

This goto chain seems odd.

>      }
>  
> +    blk2 = blk_new("image 2", &error_abort);
>      bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
>      if (!bs2) {
>          error_report("Can't open file %s", filename2);
>          ret = 2;
> -        goto out2;
> +        goto out1;
>      }
>  
>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> @@ -1183,11 +1196,14 @@ static int img_compare(int argc, char **argv)
>      ret = 0;
>  
>  out:
> -    bdrv_unref(bs2);
>      qemu_vfree(buf1);
>      qemu_vfree(buf2);
> +out1:
> +    bdrv_unref(bs2);
> +    blk_unref(blk2);
>  out2:
>      bdrv_unref(bs1);
> +    blk_unref(blk1);
>  out3:
>      qemu_progress_end();
>      return ret;
> @@ -1200,6 +1216,7 @@ static int img_convert(int argc, char **argv)
>      int progress = 0, flags, src_flags;
>      const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
>      BlockDriver *drv, *proto_drv;
> +    BlockBackend **blk = NULL, *out_blk = NULL;

Should blk be blks or blkes ? They are multiple.

>      BlockDriverState **bs = NULL, *out_bs = NULL;
>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>      int64_t *bs_sectors = NULL;
> @@ -1354,6 +1371,7 @@ static int img_convert(int argc, char **argv)
>  
>      qemu_progress_print(0, 100);
>  
> +    blk = g_new0(BlockBackend *, bs_n);
>      bs = g_new0(BlockDriverState *, bs_n);
>      bs_sectors = g_new(int64_t, bs_n);
>  
> @@ -1361,6 +1379,7 @@ static int img_convert(int argc, char **argv)
>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
>          char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
>                              : g_strdup("source");
> +        blk[bs_i] = blk_new(id, &error_abort);
>          bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
>                                   true, quiet);
>          g_free(id);
> @@ -1486,6 +1505,7 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }
>  
> +    out_blk = blk_new("target", &error_abort);
>      out_bs = bdrv_new_open("target", out_filename, out_fmt, flags, true, quiet);
>      if (!out_bs) {
>          ret = -1;
> @@ -1742,6 +1762,7 @@ out:
>      if (out_bs) {
>          bdrv_unref(out_bs);
>      }
> +    blk_unref(out_blk);
>      if (bs) {
>          for (bs_i = 0; bs_i < bs_n; bs_i++) {
>              if (bs[bs_i]) {
> @@ -1750,6 +1771,12 @@ out:
>          }
>          g_free(bs);
>      }
> +    if (blk) {
> +        for (bs_i = 0; bs_i < bs_n; bs_i++) {
> +            blk_unref(blk[bs_i]);
> +        }
> +        g_free(blk);
> +    }
>      g_free(bs_sectors);
>  fail_getopt:
>      g_free(options);
> @@ -1858,6 +1885,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>      filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
>  
>      while (filename) {
> +        BlockBackend *blk;
>          BlockDriverState *bs;
>          ImageInfo *info;
>          ImageInfoList *elem;
> @@ -1869,9 +1897,11 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>          }
>          g_hash_table_insert(filenames, (gpointer)filename, NULL);
>  
> +        blk = blk_new("image", &error_abort);
>          bs = bdrv_new_open("image", filename, fmt,
>                             BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
>          if (!bs) {
> +            blk_unref(blk);
>              goto err;
>          }
>  
> @@ -1880,6 +1910,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>              error_report("%s", error_get_pretty(err));
>              error_free(err);
>              bdrv_unref(bs);
> +            blk_unref(blk);
>              goto err;
>          }
>  
> @@ -1889,6 +1920,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>          last = &elem->next;
>  
>          bdrv_unref(bs);
> +        blk_unref(blk);
>  
>          filename = fmt = NULL;
>          if (chain) {
> @@ -2082,6 +2114,7 @@ static int img_map(int argc, char **argv)
>  {
>      int c;
>      OutputFormat output_format = OFORMAT_HUMAN;
> +    BlockBackend *blk;
>      BlockDriverState *bs;
>      const char *filename, *fmt, *output;
>      int64_t length;
> @@ -2130,9 +2163,11 @@ static int img_map(int argc, char **argv)
>          return 1;
>      }
>  
> +    blk = blk_new("image", &error_abort);
>      bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
>      if (!bs) {
> -        return 1;
> +        ret = -1;


> +        goto out;
>      }
>  
>      if (output_format == OFORMAT_HUMAN) {
> @@ -2175,6 +2210,7 @@ static int img_map(int argc, char **argv)
>  
>  out:
>      bdrv_unref(bs);
> +    blk_unref(blk);
>      return ret < 0;
>  }
>  
> @@ -2185,6 +2221,7 @@ out:
>  
>  static int img_snapshot(int argc, char **argv)
>  {
> +    BlockBackend *blk;
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn;
>      char *filename, *snapshot_name = NULL;
> @@ -2250,9 +2287,11 @@ static int img_snapshot(int argc, char **argv)
>      filename = argv[optind++];
>  
>      /* Open the image */
> +    blk = blk_new("image", &error_abort);
>      bs = bdrv_new_open("image", filename, NULL, bdrv_oflags, true, quiet);
>      if (!bs) {
> -        return 1;
> +        ret = -1;
> +        goto out;
>      }
>  
>      /* Perform the requested action */
> @@ -2296,7 +2335,9 @@ static int img_snapshot(int argc, char **argv)
>      }
>  
>      /* Cleanup */
> +out:
>      bdrv_unref(bs);
> +    blk_unref(blk);
>      if (ret) {
>          return 1;
>      }
> @@ -2305,6 +2346,7 @@ static int img_snapshot(int argc, char **argv)
>  
>  static int img_rebase(int argc, char **argv)
>  {
> +    BlockBackend *blk = NULL, *blk_old_backing = NULL, *blk_new_backing = NULL;
>      BlockDriverState *bs = NULL, *bs_old_backing = NULL, *bs_new_backing = NULL;
>      BlockDriver *old_backing_drv, *new_backing_drv;
>      char *filename;
> @@ -2393,6 +2435,7 @@ static int img_rebase(int argc, char **argv)
>       * Ignore the old backing file for unsafe rebase in case we want to correct
>       * the reference to a renamed or moved backing file.
>       */
> +    blk = blk_new("image", &error_abort);
>      bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
>      if (!bs) {
>          ret = -1;
> @@ -2425,6 +2468,7 @@ static int img_rebase(int argc, char **argv)
>      if (!unsafe) {
>          char backing_name[1024];
>  
> +        blk_old_backing = blk_new("old_backing", &error_abort);
>          bs_old_backing = bdrv_new_root("old_backing", &error_abort);
>          bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>          ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, src_flags,
> @@ -2436,6 +2480,7 @@ static int img_rebase(int argc, char **argv)
>              goto out;
>          }
>          if (out_baseimg[0]) {
> +            blk_new_backing = blk_new("new_backing", &error_abort);
>              bs_new_backing = bdrv_new_root("new_backing", &error_abort);
>              ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, src_flags,
>                              new_backing_drv, &local_err);
> @@ -2614,12 +2659,15 @@ out:
>          if (bs_old_backing != NULL) {
>              bdrv_unref(bs_old_backing);
>          }
> +        blk_unref(blk_old_backing);
>          if (bs_new_backing != NULL) {
>              bdrv_unref(bs_new_backing);
>          }
> +        blk_unref(blk_new_backing);
>      }
>  
>      bdrv_unref(bs);
> +    blk_unref(blk);
>      if (ret) {
>          return 1;
>      }
> @@ -2632,6 +2680,7 @@ static int img_resize(int argc, char **argv)
>      const char *filename, *fmt, *size;
>      int64_t n, total_size;
>      bool quiet = false;
> +    BlockBackend *blk = NULL;
>      BlockDriverState *bs = NULL;
>      QemuOpts *param;
>      static QemuOptsList resize_options = {
> @@ -2708,6 +2757,7 @@ static int img_resize(int argc, char **argv)
>      n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
>      qemu_opts_del(param);
>  
> +    blk = blk_new("image", &error_abort);
>      bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
>                         true, quiet);
>      if (!bs) {
> @@ -2745,6 +2795,7 @@ out:
>      if (bs) {
>          bdrv_unref(bs);
>      }
> +    blk_unref(blk);
>      if (ret) {
>          return 1;
>      }
> @@ -2760,6 +2811,7 @@ static int img_amend(int argc, char **argv)
>      const char *fmt = NULL, *filename, *cache;
>      int flags;
>      bool quiet = false;
> +    BlockBackend *blk = NULL;
>      BlockDriverState *bs = NULL;
>  
>      cache = BDRV_DEFAULT_CACHE;
> @@ -2823,6 +2875,7 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>  
> +    blk = blk_new("image", &error_abort);
>      bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
>      if (!bs) {
>          error_report("Could not open image '%s'", filename);
> @@ -2856,6 +2909,7 @@ out:
>      if (bs) {
>          bdrv_unref(bs);
>      }
> +    blk_unref(blk);
>      qemu_opts_del(opts);
>      qemu_opts_free(create_opts);
>      g_free(options);
> diff --git a/qemu-io.c b/qemu-io.c
> index 24ca59c..57090de 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -19,6 +19,7 @@
>  #include "qemu/option.h"
>  #include "qemu/config-file.h"
>  #include "qemu/readline.h"
> +#include "sysemu/block-backend.h"
>  #include "block/block_int.h"
>  #include "trace/control.h"
>  
> @@ -26,6 +27,7 @@
>  
>  static char *progname;
>  
> +static BlockBackend *qemuio_blk;
>  static BlockDriverState *qemuio_bs;
>  
>  /* qemu-io commands passed using -c */
> @@ -37,7 +39,9 @@ static ReadLineState *readline_state;
>  static int close_f(BlockDriverState *bs, int argc, char **argv)
>  {
>      bdrv_unref(bs);
> +    blk_unref(qemuio_blk);
>      qemuio_bs = NULL;
> +    qemuio_blk = NULL;
>      return 0;
>  }
>  
> @@ -58,6 +62,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>          return 1;
>      }
>  
> +    qemuio_blk = blk_new("hda", &error_abort);
>      qemuio_bs = bdrv_new_root("hda", &error_abort);
>  
>      if (growable) {
> @@ -70,7 +75,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>                  error_get_pretty(local_err));
>          error_free(local_err);
>          bdrv_unref(qemuio_bs);
> +        blk_unref(qemuio_blk);
>          qemuio_bs = NULL;
> +        qemuio_blk = NULL;
>          return 1;
>      }
>  
> @@ -479,6 +486,7 @@ int main(int argc, char **argv)
>      if (qemuio_bs) {
>          bdrv_unref(qemuio_bs);
>      }
> +    blk_unref(qemuio_blk);
>      g_free(readline_state);
>      return 0;
>  }
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 24b8454..ff95da6 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -17,7 +17,7 @@
>   */
>  
>  #include "qemu-common.h"
> -#include "block/block.h"
> +#include "sysemu/block-backend.h"
>  #include "block/block_int.h"
>  #include "block/nbd.h"
>  #include "qemu/main-loop.h"
> @@ -384,6 +384,7 @@ static void nbd_accept(void *opaque)
>  
>  int main(int argc, char **argv)
>  {
> +    BlockBackend *blk;
>      BlockDriverState *bs;
>      BlockDriver *drv;
>      off_t dev_offset = 0;
> @@ -687,6 +688,7 @@ int main(int argc, char **argv)
>          drv = NULL;
>      }
>  
> +    blk = blk_new("hda", &error_abort);
>      bs = bdrv_new_root("hda", &error_abort);
>  
>      srcpath = argv[optind];
> @@ -770,6 +772,7 @@ int main(int argc, char **argv)
>      } while (state != TERMINATED);
>  
>      bdrv_unref(bs);
> +    blk_unref(blk);
>      if (sockpath) {
>          unlink(sockpath);
>      }
> -- 
> 1.9.3
>