On 13.10.2015 17:37, Kevin Wolf wrote:
> 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 <mre...@redhat.com>
>
>> @@ -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.
Probably some kind of a rebase conflict. blockdev-backup was added only
rather recently, and so I made a different decision then than for
drive-backup.
I think I'll drop the check from drive_backup_prepare(). Having it in
qmp_drive_backup() is enough.
> 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.
I'd mainly argue that this function is to be removed anyway. :-)
I should just return immediately for NULL bs, as you're suggesting.
>
>> 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! ;-)
*cough cough*
>
>> @@ -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?
Yes, will do.
>> + } 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?
Because that would mean more lines modified. Every line git blame
associates with me is a line Markus might ask me something about.
I'll put it into a single block.
As always, thanks for reviewing!
Max
signature.asc
Description: OpenPGP digital signature
