Re: [Qemu-block] [PATCH v6 23/39] block: Prepare for NULL BDS

[Kevin Wolf]

Am 12.10.2015 um 22:00 hat Max Reitz geschrieben:
> blk_bs() will not necessarily return a non-NULL value any more (unless
> blk_is_available() is true or it can be assumed to otherwise, e.g.
> because it is called immediately after a successful blk_new_with_bs() or
> blk_new_open()).
> 
> Signed-off-by: Max Reitz <address@hidden>

> @@ -1584,12 +1594,16 @@ static void drive_backup_prepare(BlkTransactionState 
> *common, Error **errp)
>                    "Device '%s' not found", backup->device);
>          return;
>      }
> -    bs = blk_bs(blk);
>  
>      /* AioContext is released in .clean() */
> -    state->aio_context = bdrv_get_aio_context(bs);
> +    state->aio_context = blk_get_aio_context(blk);
>      aio_context_acquire(state->aio_context);
>  
> +    if (!blk_is_available(blk)) {
> +        error_setg(errp, "Device '%s' has no medium", backup->device);
> +        return;
> +    }
> +
>      qmp_drive_backup(backup->device, backup->target,
>                       backup->has_format, backup->format,
>                       backup->sync,
> @@ -1604,7 +1618,7 @@ static void drive_backup_prepare(BlkTransactionState 
> *common, Error **errp)
>          return;
>      }
>  
> -    state->bs = bs;
> +    state->bs = blk_bs(blk);
>      state->job = state->bs->job;
>  }
>  
> @@ -1639,8 +1653,7 @@ static void blockdev_backup_prepare(BlkTransactionState 
> *common, Error **errp)
>  {
>      BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
> common);
>      BlockdevBackup *backup;
> -    BlockDriverState *bs, *target;
> -    BlockBackend *blk;
> +    BlockBackend *blk, *target;
>      Error *local_err = NULL;
>  
>      assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> @@ -1651,18 +1664,16 @@ static void 
> blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
>          error_setg(errp, "Device '%s' not found", backup->device);
>          return;
>      }
> -    bs = blk_bs(blk);
>  
> -    blk = blk_by_name(backup->target);
> -    if (!blk) {
> +    target = blk_by_name(backup->target);
> +    if (!target) {
>          error_setg(errp, "Device '%s' not found", backup->target);
>          return;
>      }
> -    target = blk_bs(blk);
>  
>      /* AioContext is released in .clean() */
> -    state->aio_context = bdrv_get_aio_context(bs);
> -    if (state->aio_context != bdrv_get_aio_context(target)) {
> +    state->aio_context = blk_get_aio_context(blk);
> +    if (state->aio_context != blk_get_aio_context(target)) {
>          state->aio_context = NULL;
>          error_setg(errp, "Backup between two IO threads is not implemented");
>          return;
> @@ -1680,7 +1691,7 @@ static void blockdev_backup_prepare(BlkTransactionState 
> *common, Error **errp)
>          return;
>      }
>  
> -    state->bs = bs;
> +    state->bs = blk_bs(blk);
>      state->job = state->bs->job;
>  }

It's somewhat inconsistent that blockdev_backup_prepare() doesn't do an
explicit blk_is_available() check whereas drive_backup_prepare() does.
As far as I can tell, both don't necessarily need their for their own
code and in both cases the called qmp_*_backup() function checks it
again.

Not really a problem, though, it just looks a bit odd.

> @@ -1818,10 +1829,10 @@ static void eject_device(BlockBackend *blk, int 
> force, Error **errp)
>      BlockDriverState *bs = blk_bs(blk);
>      AioContext *aio_context;
>  
> -    aio_context = bdrv_get_aio_context(bs);
> +    aio_context = blk_get_aio_context(blk);
>      aio_context_acquire(aio_context);
>  
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
> +    if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
>          goto out;
>      }
>      if (!blk_dev_has_removable_media(blk)) {
>          error_setg(errp, "Device '%s' is not removable",
>                     bdrv_get_device_name(bs));

bs isn't checked to be non-NULL here. You could argue that if it's not
removable, it shouldn't be NULL, but I think we let drive_del forcibly
remove the medium even for devices that can't deal with it and start
producing I/O errors then.

>          goto out;
>      }
>
>      if (blk_dev_is_medium_locked(blk) && !blk_dev_is_tray_open(blk)) {
>          blk_dev_eject_request(blk, force);
>          if (!force) {
>              error_setg(errp, "Device '%s' is locked",
>                         bdrv_get_device_name(bs));

And here.

>              goto out;
>          }
>      }
>  
> -    bdrv_close(bs);
> +    if (bs) {
> +        bdrv_close(bs);
> +    }
>  
>  out:
>      aio_context_release(aio_context);

I think the change becomes simpler if you just return immediately for a
NULL bs instead of evaluation for every place whether checking it could
make a difference.

> @@ -1884,10 +1897,12 @@ void qmp_block_passwd(bool has_device, const char 
> *device,
>  }
>  
>  /* Assumes AioContext is held */
> -static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char 
> *filename,
> +static void qmp_bdrv_open_encrypted(BlockDriverState **pbs,
> +                                    const char *filename,
>                                      int bdrv_flags, const char *format,
>                                      const char *password, Error **errp)
>  {
> +    BlockDriverState *bs;
>      Error *local_err = NULL;
>      QDict *options = NULL;
>      int ret;
> @@ -1897,11 +1912,12 @@ static void qmp_bdrv_open_encrypted(BlockDriverState 
> *bs, const char *filename,
>          qdict_put(options, "driver", qstring_from_str(format));
>      }
>  
> -    ret = bdrv_open(&bs, filename, NULL, options, bdrv_flags, &local_err);
> +    ret = bdrv_open(pbs, filename, NULL, options, bdrv_flags, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
>      }
> +    bs = *pbs;
>  
>      bdrv_add_key(bs, password, errp);
>  }

Yup, we definitely need a local variable bs for this one line! ;-)

> @@ -1913,6 +1929,7 @@ void qmp_change_blockdev(const char *device, const char 
> *filename,
>      BlockDriverState *bs;
>      AioContext *aio_context;
>      int bdrv_flags;
> +    bool new_bs;
>      Error *err = NULL;
>  
>      blk = blk_by_name(device);
> @@ -1922,8 +1939,9 @@ void qmp_change_blockdev(const char *device, const char 
> *filename,
>          return;
>      }
>      bs = blk_bs(blk);
> +    new_bs = !bs;
>  
> -    aio_context = bdrv_get_aio_context(bs);
> +    aio_context = blk_get_aio_context(blk);
>      aio_context_acquire(aio_context);
>  
>      eject_device(blk, 0, &err);
> @@ -1932,10 +1950,18 @@ void qmp_change_blockdev(const char *device, const 
> char *filename,
>          goto out;
>      }
>  
> -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> +    bdrv_flags = blk_is_read_only(blk) ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= blk_get_root_state(blk)->open_flags & ~BDRV_O_RDWR;
>  
> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, format, NULL, errp);
> +    qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, &err);
> +    if (err) {
> +        error_propagate(errp, err);

goto out just to keep working when someone adds another line of code
before out?

> +    } else if (new_bs) {
> +        blk_insert_bs(blk, bs);
> +        /* Has been sent automatically by bdrv_open() if blk_bs(blk) was not
> +         * NULL */
> +        blk_dev_change_media_cb(blk, true);
> +    }
>  
>  out:
>      aio_context_release(aio_context);

> @@ -2151,16 +2182,19 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>          return;
>      }
>  
> -    aio_context = bdrv_get_aio_context(bs);
> +    aio_context = blk_get_aio_context(blk);
>      aio_context_acquire(aio_context);
>  
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
> +    bs = blk_bs(blk);
> +    if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
>          error_report_err(local_err);
>          aio_context_release(aio_context);
>          return;
>      }
>  
> -    bdrv_close(bs);
> +    if (bs) {
> +        bdrv_close(bs);
> +    }

Why not a single if (bs) block?

Kevin