[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 23/39] block: Prepare for NULL BDS
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v6 23/39] block: Prepare for NULL BDS |
Date: |
Tue, 13 Oct 2015 17:37:27 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- Re: [Qemu-block] [PATCH v6 18/39] block: Add BlockBackendRootState, (continued)
[Qemu-block] [PATCH v6 19/39] block: Make some BB functions fall back to BBRS, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 20/39] block: Fail requests to empty BlockBackend, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 21/39] block: Prepare remaining BB functions for NULL BDS, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 22/39] block: Add blk_insert_bs(), Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 24/39] blockdev: Do not create BDS for empty drive, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 23/39] block: Prepare for NULL BDS, Max Reitz, 2015/10/12
- Re: [Qemu-block] [PATCH v6 23/39] block: Prepare for NULL BDS,
Kevin Wolf <=
[Qemu-block] [PATCH v6 25/39] blockdev: Pull out blockdev option extraction, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 26/39] blockdev: Allow more options for BB-less BDS tree, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 29/39] blockdev: Add blockdev-close-tray, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 32/39] blockdev: Implement eject with basic operations, Max Reitz, 2015/10/12
[Qemu-block] [PATCH v6 30/39] blockdev: Add blockdev-remove-medium, Max Reitz, 2015/10/12