Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static

[Kevin Wolf <kwolf@redhat.com>]

Am 31.01.2014 um 21:20 hat Max Reitz geschrieben:
> On 29.01.2014 14:26, Kevin Wolf wrote:
> >Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> >>Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
> >>call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
> >>therefore bdrv_open() the only way to call it.
> >>
> >>Consequently, all existing calls to bdrv_file_open() have to be adjusted
> >>to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  block.c               | 17 ++++++++++++-----
> >>  block/cow.c           |  6 +++---
> >>  block/qcow.c          |  6 +++---
> >>  block/qcow2.c         |  5 +++--
> >>  block/qed.c           |  5 +++--
> >>  block/sheepdog.c      |  8 +++++---
> >>  block/vhdx.c          |  5 +++--
> >>  block/vmdk.c          | 11 +++++++----
> >>  include/block/block.h |  5 ++---
> >>  qemu-io.c             |  4 +++-
> >>  10 files changed, 44 insertions(+), 28 deletions(-)

> >>diff --git a/include/block/block.h b/include/block/block.h
> >>index a421041..396f9ed 100644
> >>--- a/include/block/block.h
> >>+++ b/include/block/block.h
> >>@@ -102,6 +102,8 @@ typedef enum {
> >>  #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
> >>  #define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w */
> >>  #define BDRV_O_UNMAP       0x4000  /* execute guest UNMAP/TRIM operations */
> >>+#define BDRV_O_PROTOCOL    0x8000  /* open the file using a protocol instead of
> >>+                                      a block driver */
> >Protocol drivers are a subset of block drivers, so this description
> >doesn't make sense.
> 
> Hm, technically they probably are but it always seemed to me that
> bdrv_open() would never directly use a protocol, instead using raw
> as the format if no format was found.

Except if you explicitly specify something like format=file.

> More importantly, it is actually possible to use a non-protocol
> block driver with BDRV_O_PROTOCOL; it just needs to be explicitly
> specified.

Yes, I think with explicit specification of the driver you get mostly
the same results with bdrv_open() and bdrv_file_open().

> >I guess we need to list the differences between bdrv_open() and
> >bdrv_file_open() in order to define what this flag really changes. I
> >think this includes:
> >
> >- Disables format probing
> >- BDRV_O_SNAPSHOT is ignored
> >- No backing files are opened
> >- Probably a few more
> 
> So, to me, the main difference is that bdrv_open() always uses some
> non-protocol block driver, whereas bdrv_file_open() only probes for
> protocol block drivers (if a non-protocol driver should be used, it
> has to be explicitly specified).
> 
> The current comment for bdrv_file_open() doesn't help much, either:
> “Opens a file using a protocol”. Therefore, the easiest way would
> just be to state “behaves like the old bdrv_file_open()”, but that
> would not be very helpful. Perhaps we could formulate it like “Opens
> a single file (no backing chain, etc.) using only a protocol driver
> deduced from the filename, if not explicitly specified otherwise.”

Perhaps we should really specify it as the difference in probing:
- bdrv_open() adds a probed format layer on top of bs
- bdrv_file_open() probes protocols for bs

All other differences can probably go away without breaking anything:
BDRV_O_SNAPSHOT can be hopefully be moved to drive_init() (the tricky one
here is qmp_change_blockdev), and supporting backing files in
bdrv_file_open() by moving their handling to common code shouldn't hurt.

Any other differences that need to be eliminated? Once we know
what the differences should be, I guess we can greatly simplify the
implementation.

Kevin