Re: [Qemu-devel] [PATCH v2 00/14] block: Remove "growable", add blk_new_open()

[Max Reitz]

On 2015-01-26 at 07:18, Stefano Stabellini wrote:
> On Thu, 22 Jan 2015, Max Reitz wrote:
> > This series removes the "growable" field from the BlockDriverState
> > object. Its use was to clamp guest requests against the limits of the
> > BDS; however, this can now be done more easily by moving those checks
> > into the BlockBackend functions.
> >
> > In a future series, "growable" may be reintroduced (maybe with a
> > different name); it will then signify whether a BDS is able to grow (in
> > contrast to the current "growable", which signifies whether it is
> > allowed to). Maybe I will add it to the BlockDriver instead of the BDS,
> > though.
> >
> > To be able to remove that field, qemu-io needs to be converted to
> > BlockBackend, which is done by this series as well. While working on
> > that I decided to convert blk_new_with_bs()+bdrv_open() to
> > blk_new_open(). I was skeptical about that decision at first, but it
> > seems good now that I was able to replace nearly every blk_new_with_bs()
> > call by blk_new_open(). In a future series I may try to convert some
> > remaining bdrv_open() calls to blk_new_open() as well. (And, in fact, in
> > a future series I *will* replace the last remaining blk_new_with_bs()
> > outside of blk_new_open() by blk_new_open().)
> >
> > Finally, the question needs to be asked: If, after this series, every
> > BDS is allowed to grow, are there any users which do not use BB, but
> > should still be disallowed from reading/writing beyond a BDS's limits?
> > The only users I could see were the block jobs. Some of them should
> > indeed be converted to BB; but none of them takes a user-supplied offset
> > or size, all work on the full BDS (or only on parts which have been
> > modified, etc.). Therefore, it is by design impossible for them to
> > exceed the BDS's limits, which makes making all BDS's growable safe.
> Hello Max,
> I applied the first four patches in this series, and this is what I get:
>
> hw/block/xen_disk.c: In function ‘blk_connect’:
> hw/block/xen_disk.c:907:27: error: implicit declaration of function 
> ‘qstring_from_str’ [-Werror=implicit-function-declaration]
> hw/block/xen_disk.c:907:27: error: nested extern declaration of 
> ‘qstring_from_str’ [-Werror=nested-externs]
> hw/block/xen_disk.c:907:27: error: invalid type argument of ‘->’ (have ‘int’)
> hw/block/xen_disk.c:901:22: error: unused variable ‘drv’ 
> [-Werror=unused-variable]
> hw/block/xen_disk.c:900:23: error: unused variable ‘blk’ 
> [-Werror=unused-variable]
>
> it would be great if you could build test it.

Sorry, of course I build test my patches, but I just don't have some 
subsystems enabled (there was no real reason not to have the Xen 
backends enabled, but that's just how it was; another example are 
Windows-specific areas), so I have to work blind on them. In this case I 
forgot to include the QMP type headers and to remove the now unused 
variables.

Thank you for looking at my patches and finding this issue! I'll send a 
fixed v3.

Max

> > v2:
> > - Rebased [Kevin]
> > - Patch 2: Added a TODO comment about removing @filename and @flags from
> >    blk_new_open() when possible [Kevin]
> >
> >
> > git-backport-diff against v1:
> >
> > Key:
> > [----] : patches are identical
> > [####] : number of functional differences between upstream/downstream patch
> > [down] : patch is downstream-only
> > The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> >
> > 001/14:[----] [--] 'block: Lift some BDS functions to the BlockBackend'
> > 002/14:[0006] [FC] 'block: Add blk_new_open()'
> > 003/14:[----] [-C] 'blockdev: Use blk_new_open() in blockdev_init()'
> > 004/14:[----] [--] 'block/xen: Use blk_new_open() in blk_connect()'
> > 005/14:[----] [--] 'qemu-img: Use blk_new_open() in img_open()'
> > 006/14:[----] [--] 'qemu-img: Use blk_new_open() in img_rebase()'
> > 007/14:[----] [--] 'qemu-img: Use BlockBackend as far as possible'
> > 008/14:[----] [--] 'qemu-nbd: Use blk_new_open() in main()'
> > 009/14:[----] [--] 'qemu-io: Use blk_new_open() in openfile()'
> > 010/14:[0002] [FC] 'qemu-io: Remove "growable" option'
> > 011/14:[----] [--] 'qemu-io: Use BlockBackend'
> > 012/14:[----] [--] 'block: Clamp BlockBackend requests'
> > 013/14:[----] [--] 'block: Remove "growable" from BDS'
> > 014/14:[----] [--] 'block: Keep bdrv_check*_request()'s return value'
> >
> >
> > Max Reitz (14):
> >    block: Lift some BDS functions to the BlockBackend
> >    block: Add blk_new_open()
> >    blockdev: Use blk_new_open() in blockdev_init()
> >    block/xen: Use blk_new_open() in blk_connect()
> >    qemu-img: Use blk_new_open() in img_open()
> >    qemu-img: Use blk_new_open() in img_rebase()
> >    qemu-img: Use BlockBackend as far as possible
> >    qemu-nbd: Use blk_new_open() in main()
> >    qemu-io: Use blk_new_open() in openfile()
> >    qemu-io: Remove "growable" option
> >    qemu-io: Use BlockBackend
> >    block: Clamp BlockBackend requests
> >    block: Remove "growable" from BDS
> >    block: Keep bdrv_check*_request()'s return value
> >
> >   block.c                        |  59 +++++-----
> >   block/block-backend.c          | 219 +++++++++++++++++++++++++++++++++++++
> >   block/qcow2.c                  |   6 --
> >   block/raw-posix.c              |   2 +-
> >   block/raw-win32.c              |   2 +-
> >   block/sheepdog.c               |   2 +-
> >   blockdev.c                     |  92 ++++++++--------
> >   hmp.c                          |   9 +-
> >   hw/block/xen_disk.c            |  24 ++---
> >   include/block/block_int.h      |   3 -
> >   include/qemu-io.h              |   4 +-
> >   include/sysemu/block-backend.h |  12 +++
> >   qemu-img.c                     | 171 ++++++++++++++---------------
> >   qemu-io-cmds.c                 | 238 ++++++++++++++++++++++-------------------
> >   qemu-io.c                      |  58 ++++------
> >   qemu-nbd.c                     |  25 ++---
> >   tests/qemu-iotests/016         |  73 -------------
> >   tests/qemu-iotests/016.out     |  23 ----
> >   tests/qemu-iotests/051.out     |  60 +++++------
> >   tests/qemu-iotests/087.out     |   8 +-
> >   tests/qemu-iotests/group       |   1 -
> >   21 files changed, 590 insertions(+), 501 deletions(-)
> >   delete mode 100755 tests/qemu-iotests/016
> >   delete mode 100644 tests/qemu-iotests/016.out
> >
> > --
> > 2.1.0