From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38909) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9KYJ-0007ih-On for qemu-devel@nongnu.org; Fri, 31 Jan 2014 15:19:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W9KYD-0002vq-Nm for qemu-devel@nongnu.org; Fri, 31 Jan 2014 15:19:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50805) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9KYD-0002vb-GK for qemu-devel@nongnu.org; Fri, 31 Jan 2014 15:18:57 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0VKIuaq028090 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 31 Jan 2014 15:18:56 -0500 Message-ID: <52EC059B.10209@redhat.com> Date: Fri, 31 Jan 2014 21:20:43 +0100 From: Max Reitz MIME-Version: 1.0 References: <1390762963-25538-1-git-send-email-mreitz@redhat.com> <1390762963-25538-4-git-send-email-mreitz@redhat.com> <20140129132605.GG2726@dhcp-200-207.str.redhat.com> In-Reply-To: <20140129132605.GG2726@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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 th= e >> call to bdrv_file_open(). Additionally, make bdrv_file_open() static a= nd >> therefore bdrv_open() the only way to call it. >> >> Consequently, all existing calls to bdrv_file_open() have to be adjust= ed >> to use bdrv_open() with the BDRV_O_PROTOCOL flag instead. >> >> Signed-off-by: Max Reitz >> --- >> 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/block.c b/block.c >> index f9923bb..0fb7892 100644 >> --- a/block.c >> +++ b/block.c >> @@ -947,9 +947,9 @@ free_and_fail: >> * after the call (even on failure), so if the caller intends to reu= se the >> * dictionary, it needs to use QINCREF() before calling bdrv_file_op= en. >> */ >> -int bdrv_file_open(BlockDriverState **pbs, const char *filename, >> - const char *reference, QDict *options, int flags, >> - Error **errp) >> +static int bdrv_file_open(BlockDriverState **pbs, const char *filenam= e, >> + const char *reference, QDict *options, int = flags, >> + Error **errp) >> { >> BlockDriverState *bs =3D NULL; >> BlockDriver *drv; >> @@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, cons= t char *filename, >> *pbs =3D NULL; >> ret =3D bdrv_open(pbs, filename, NULL, image_options, flags,= NULL, errp); >> } else { >> - ret =3D bdrv_file_open(pbs, filename, reference, image_option= s, flags, >> - errp); >> + *pbs =3D NULL; >> + ret =3D bdrv_open(pbs, filename, reference, image_options, >> + flags | BDRV_O_PROTOCOL, NULL, errp); >> } >> =20 >> done: >> @@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const cha= r *filename, >> const char *drvname; >> Error *local_err =3D NULL; >> =20 >> + if (flags & BDRV_O_PROTOCOL) { >> + assert(!drv); >> + return bdrv_file_open(pbs, filename, reference, options, >> + flags & ~BDRV_O_PROTOCOL, errp); >> + } >> + >> /* NULL means an empty set of options */ >> if (options =3D=3D NULL) { >> options =3D qdict_new(); >> diff --git a/block/cow.c b/block/cow.c >> index 7fc0b12..f0748a2 100644 >> --- a/block/cow.c >> +++ b/block/cow.c >> @@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOp= tionParameter *options, >> const char *image_filename =3D NULL; >> Error *local_err =3D NULL; >> int ret; >> - BlockDriverState *cow_bs; >> + BlockDriverState *cow_bs =3D NULL; >> =20 >> /* Read out options */ >> while (options && options->name) { >> @@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOp= tionParameter *options, >> return ret; >> } >> =20 >> - ret =3D bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR= , >> - &local_err); >> + ret =3D bdrv_open(&cow_bs, filename, NULL, NULL, >> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> diff --git a/block/qcow.c b/block/qcow.c >> index 948b0c5..4881d84 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUO= ptionParameter *options, >> int flags =3D 0; >> Error *local_err =3D NULL; >> int ret; >> - BlockDriverState *qcow_bs; >> + BlockDriverState *qcow_bs =3D NULL; >> =20 >> /* Read out options */ >> while (options && options->name) { >> @@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUO= ptionParameter *options, >> return ret; >> } >> =20 >> - ret =3D bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDW= R, >> - &local_err); >> + ret =3D bdrv_open(&qcow_bs, filename, NULL, NULL, >> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 7202a26..40b957b 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -1479,7 +1479,7 @@ static int qcow2_create2(const char *filename, i= nt64_t total_size, >> * 2 GB for 64k clusters, and we don't want to have a 2 GB initi= al file >> * size for any qcow2 image. >> */ >> - BlockDriverState* bs; >> + BlockDriverState *bs =3D NULL; >> QCowHeader *header; >> uint8_t* refcount_table; >> Error *local_err =3D NULL; >> @@ -1491,7 +1491,8 @@ static int qcow2_create2(const char *filename, i= nt64_t total_size, >> return ret; >> } >> =20 >> - ret =3D bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &l= ocal_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O= _PROTOCOL, >> + NULL, &local_err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> return ret; >> diff --git a/block/qed.c b/block/qed.c >> index 694e6e2..dee2e61 100644 >> --- a/block/qed.c >> +++ b/block/qed.c >> @@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32= _t cluster_size, >> return ret; >> } >> =20 >> - ret =3D bdrv_file_open(&bs, filename, NULL, NULL, >> - BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, >> + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, = NULL, >> + &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 672b9c9..6f2ec57 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename) >> Error *local_err =3D NULL; >> int ret; >> =20 >> - ret =3D bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &l= ocal_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O= _PROTOCOL, >> + NULL, &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> @@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUO= ptionParameter *options, >> } >> =20 >> if (backing_file) { >> - BlockDriverState *bs; >> + BlockDriverState *bs =3D NULL; >> BDRVSheepdogState *base; >> BlockDriver *drv; >> =20 >> @@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUO= ptionParameter *options, >> goto out; >> } >> =20 >> - ret =3D bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &loc= al_err); >> + ret =3D bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTO= COL, NULL, >> + &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> diff --git a/block/vhdx.c b/block/vhdx.c >> index 9ee0a61..13513b4 100644 >> --- a/block/vhdx.c >> +++ b/block/vhdx.c >> @@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEM= UOptionParameter *options, >> =20 >> gunichar2 *creator =3D NULL; >> glong creator_items; >> - BlockDriverState *bs; >> + BlockDriverState *bs =3D NULL; >> const char *type =3D NULL; >> VHDXImageType image_type; >> Error *local_err =3D NULL; >> @@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEM= UOptionParameter *options, >> goto exit; >> } >> =20 >> - ret =3D bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &l= ocal_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O= _PROTOCOL, >> + NULL, &local_err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> goto exit; >> diff --git a/block/vmdk.c b/block/vmdk.c >> index 37b2bc8..d2a69d5 100644 >> --- a/block/vmdk.c >> +++ b/block/vmdk.c >> @@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, Bl= ockDriverState *bs, >> =20 >> path_combine(extent_path, sizeof(extent_path), >> desc_file_path, fname); >> - ret =3D bdrv_file_open(&extent_file, extent_path, NULL, NULL, >> - bs->open_flags, errp); >> + extent_file =3D NULL; >> + ret =3D bdrv_open(&extent_file, extent_path, NULL, NULL, >> + bs->open_flags | BDRV_O_PROTOCOL, NULL, errp)= ; >> if (ret) { >> return ret; >> } >> @@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filena= me, int64_t filesize, >> goto exit; >> } >> =20 >> - ret =3D bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &l= ocal_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O= _PROTOCOL, >> + NULL, &local_err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> goto exit; >> @@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEM= UOptionParameter *options, >> goto exit; >> } >> } >> - ret =3D bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR= , &local_err); >> + ret =3D bdrv_open(&new_bs, filename, NULL, NULL, >> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not write description"); >> goto exit; >> 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 ch= eck */ >> #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 opera= tions */ >> +#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=20 bdrv_open() would never directly use a protocol, instead using raw as=20 the format if no format was found. More importantly, it is actually possible to use a non-protocol block=20 driver with BDRV_O_PROTOCOL; it just needs to be explicitly specified. > 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=20 non-protocol block driver, whereas bdrv_file_open() only probes for=20 protocol block drivers (if a non-protocol driver should be used, it has=20 to be explicitly specified). The current comment for bdrv_file_open() doesn't help much, either:=20 =93Opens a file using a protocol=94. Therefore, the easiest way would jus= t=20 be to state =93behaves like the old bdrv_file_open()=94, but that would n= ot=20 be very helpful. Perhaps we could formulate it like =93Opens a single fil= e=20 (no backing chain, etc.) using only a protocol driver deduced from the=20 filename, if not explicitly specified otherwise.=94 Max