Kevin Wolf <kw...@redhat.com> writes:
> Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
>> Just four uses of BlockDriverState are left:
>>
>> * The Xen paravirtual block device backend (xen_disk.c) opens images
>> itself when set up via xenbus, bypassing blockdev.c. I figure it
>> should go through qmp_blockdev_add() instead.
>>
>> * Device model "usb-storage" prompts for keys. No other device model
>> does, and this one probably shouldn't do it, either.
>>
>> * ide_issue_trim_cb() uses bdrv_aio_discard() instead of
>> blk_aio_discard() because it fishes its backend out of a BlockAIOCB,
>> which has only the BlockDriverState.
>>
>> * PC87312State has an unused BlockDriverState[] member.
>>
>> The next two commits take care of the latter two.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>
> Okay. This patch gives us a chance to have a look at each of the
> functions that we'll expose in the BlockBackend layer.
Yes. I really like how BlockBackend separates the API meant for use
within the block layer from the one for its users, such as device
models.
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 7fd832d..5f796b4 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -235,3 +235,260 @@ void blk_hide_on_behalf_of_do_drive_del(BlockBackend
>> *blk)
>> bdrv_make_anon(blk->bs);
>> }
>> }
>> +
>> +void blk_iostatus_enable(BlockBackend *blk)
>> +{
>> + bdrv_iostatus_enable(blk->bs);
>> +}
>
> Even at the end of the series, iostatus (i.e. rerror/werror handling) is
> still at the BDS level. I think our earlier conclusion was that
> it's really a BB thing, so I would expect it to be implemented in
> block-backend.c with no calls into block.c.
>
> Is this on your todo list?
Definitely. Anything touched by bdrv_move_feature_fields() is a
candidate for lifting from BDS into BB.
This series lifts almost nothing.
>> +int blk_attach_dev(BlockBackend *blk, void *dev)
>> +{
>> + return bdrv_attach_dev(blk->bs, dev);
>> +}
>> +
>> +void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
>> +{
>> + bdrv_attach_dev_nofail(blk->bs, dev);
>> +}
>> +
>> +void blk_detach_dev(BlockBackend *blk, void *dev)
>> +{
>> + bdrv_detach_dev(blk->bs, dev);
>> +}
>> +
>> +void *blk_get_attached_dev(BlockBackend *blk)
>> +{
>> + return bdrv_get_attached_dev(blk->bs);
>> +}
>> +
>> +void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void
>> *opaque)
>> +{
>> + bdrv_set_dev_ops(blk->bs, ops, opaque);
>> +}
>
> At the end of the series, all of these are BB-only code. Good.
Yes. I included lifting this part to have at least one meaningful
example above and beyond the bare minimum of name.
>> +int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
>> + int nb_sectors)
>> +{
>> + return bdrv_read(blk->bs, sector_num, buf, nb_sectors);
>> +}
>> +
>> +int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t
>> *buf,
>> + int nb_sectors)
>> +{
>> + return bdrv_read_unthrottled(blk->bs, sector_num, buf, nb_sectors);
>> +}
>
> This one is a hack that I'd rather see disappear. Unrelated to this
> series, though.
Okay :)
>> +int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
>> + int nb_sectors)
>> +{
>> + return bdrv_write(blk->bs, sector_num, buf, nb_sectors);
>> +}
>> +
>> +BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
>> + int nb_sectors, BdrvRequestFlags flags,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_write_zeroes(blk->bs, sector_num, nb_sectors, flags,
>> + cb, opaque);
>> +}
>> +
>> +int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
>> +{
>> + return bdrv_pread(blk->bs, offset, buf, count);
>> +}
>> +
>> +int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int
>> count)
>> +{
>> + return bdrv_pwrite(blk->bs, offset, buf, count);
>> +}
>
> Any particular reason for the ordering of the functions here? Is this
> just how they were in block.c? I would have expected all read function
> and all write functions in one place.
Yes, same order. I'm quite happy to put them in any order you like.
>> +int64_t blk_getlength(BlockBackend *blk)
>> +{
>> + return bdrv_getlength(blk->bs);
>> +}
>> +
>> +void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr)
>> +{
>> + bdrv_get_geometry(blk->bs, nb_sectors_ptr);
>> +}
>> +
>> +BlockAIOCB *blk_aio_readv(BlockBackend *blk, int64_t sector_num,
>> + QEMUIOVector *iov, int nb_sectors,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_readv(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
>> +}
>> +
>> +BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t sector_num,
>> + QEMUIOVector *iov, int nb_sectors,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_writev(blk->bs, sector_num, iov, nb_sectors, cb,
>> opaque);
>> +}
>> +
>> +BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_flush(blk->bs, cb, opaque);
>> +}
>> +
>> +BlockAIOCB *blk_aio_discard(BlockBackend *blk,
>> + int64_t sector_num, int nb_sectors,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_discard(blk->bs, sector_num, nb_sectors, cb, opaque);
>> +}
>> +
>> +void blk_aio_cancel(BlockAIOCB *acb)
>> +{
>> + bdrv_aio_cancel(acb);
>> +}
>> +
>> +int blk_aio_multiwrite(BlockBackend *blk, BlockRequest *reqs, int num_reqs)
>> +{
>> + return bdrv_aio_multiwrite(blk->bs, reqs, num_reqs);
>> +}
>> +
>> +int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
>> +{
>> + return bdrv_ioctl(blk->bs, req, buf);
>> +}
>> +
>> +BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req,
>> void *buf,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
>> +}
>> +
>> +int blk_flush(BlockBackend *blk)
>> +{
>> + return bdrv_flush(blk->bs);
>> +}
>> +
>> +int blk_flush_all(void)
>> +{
>> + return bdrv_flush_all();
>> +}
>> +
>> +void blk_drain_all(void)
>> +{
>> + bdrv_drain_all();
>> +}
>
>> +BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read)
>> +{
>> + return bdrv_get_on_error(blk->bs, is_read);
>> +}
>> +
>> +BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
>> + int error)
>> +{
>> + return bdrv_get_error_action(blk->bs, is_read, error);
>> +}
>> +
>> +void blk_error_action(BlockBackend *blk, BlockErrorAction action,
>> + bool is_read, int error)
>> +{
>> + bdrv_error_action(blk->bs, action, is_read, error);
>> +}
>
> Like iostatus, this probably should be a BB-only thing in the end.
Like iostatus, it's touched by bdrv_move_feature_fields().
>> +int blk_is_read_only(BlockBackend *blk)
>> +{
>> + return bdrv_is_read_only(blk->bs);
>> +}
>> +
>> +int blk_is_sg(BlockBackend *blk)
>> +{
>> + return bdrv_is_sg(blk->bs);
>> +}
>> +
>> +int blk_enable_write_cache(BlockBackend *blk)
>> +{
>> + return bdrv_enable_write_cache(blk->bs);
>> +}
>> +
>> +void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
>> +{
>> + bdrv_set_enable_write_cache(blk->bs, wce);
>> +}
>> +
>> +int blk_is_inserted(BlockBackend *blk)
>> +{
>> + return bdrv_is_inserted(blk->bs);
>> +}
>> +
>> +void blk_lock_medium(BlockBackend *blk, bool locked)
>> +{
>> + bdrv_lock_medium(blk->bs, locked);
>> +}
>> +
>> +void blk_eject(BlockBackend *blk, bool eject_flag)
>> +{
>> + bdrv_eject(blk->bs, eject_flag);
>> +}
>> +
>> +int blk_get_flags(BlockBackend *blk)
>> +{
>> + return bdrv_get_flags(blk->bs);
>> +}
>> +
>> +void blk_set_guest_block_size(BlockBackend *blk, int align)
>> +{
>> + bdrv_set_guest_block_size(blk->bs, align);
>> +}
>> +
>> +void *blk_blockalign(BlockBackend *blk, size_t size)
>
> Perhaps better blk_memalign?
Well, the actual alignment comes from bdrv_opt_mem_align(blk->bs).
>> +{
>> + return qemu_blockalign(blk ? blk->bs : NULL, size);
>> +}
>> +
>> +bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
>> +{
>> + return bdrv_op_is_blocked(blk->bs, op, errp);
>> +}
>> +
>> +void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason)
>> +{
>> + bdrv_op_unblock(blk->bs, op, reason);
>> +}
>> +
>> +void blk_op_block_all(BlockBackend *blk, Error *reason)
>> +{
>> + bdrv_op_block_all(blk->bs, reason);
>> +}
>> +
>> +void blk_op_unblock_all(BlockBackend *blk, Error *reason)
>> +{
>> + bdrv_op_unblock_all(blk->bs, reason);
>> +}
>> +
>> +AioContext *blk_get_aio_context(BlockBackend *blk)
>> +{
>> + return bdrv_get_aio_context(blk->bs);
>> +}
>> +
>> +void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>> +{
>> + bdrv_set_aio_context(blk->bs, new_context);
>> +}
>> +
>> +void blk_io_plug(BlockBackend *blk)
>> +{
>> + bdrv_io_plug(blk->bs);
>> +}
>> +
>> +void blk_io_unplug(BlockBackend *blk)
>> +{
>> + bdrv_io_unplug(blk->bs);
>> +}
>> +
>> +BlockAcctStats *blk_get_stats(BlockBackend *blk)
>> +{
>> + return bdrv_get_stats(blk->bs);
>> +}
>> +
>> +void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
>> + BlockCompletionFunc *cb, void *opaque)
>> +{
>> + return qemu_aio_get(aiocb_info, blk_bs(blk), cb, opaque);
>> +}
>
> There are a few more in this list that look dubious, but this patch
> series isn't the right place to discuss them.
>
> Found nothing noteworthy in the rest of the patch, though I only
> quickly scanned it, so that wasn't thorough enough for a R-b.
Thanks anyway!
