From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm1e3-0001UO-Dn for qemu-devel@nongnu.org; Tue, 13 Oct 2015 11:37:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm1dx-0000YE-5s for qemu-devel@nongnu.org; Tue, 13 Oct 2015 11:37:43 -0400 Date: Tue, 13 Oct 2015 17:37:27 +0200 From: Kevin Wolf Message-ID: <20151013153727.GK4906@noname.str.redhat.com> References: <1444680042-13207-1-git-send-email-mreitz@redhat.com> <1444680042-13207-24-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1444680042-13207-24-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 23/39] block: Prepare for NULL BDS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi 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 > @@ -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