From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38262) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKoyw-0003pi-Bj for qemu-devel@nongnu.org; Wed, 06 Jul 2016 11:43:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKoyq-0006JS-LW for qemu-devel@nongnu.org; Wed, 06 Jul 2016 11:43:21 -0400 References: <1467732272-23368-1-git-send-email-clord@redhat.com> <1467732272-23368-18-git-send-email-clord@redhat.com> From: Max Reitz Message-ID: Date: Wed, 6 Jul 2016 17:43:03 +0200 MIME-Version: 1.0 In-Reply-To: <1467732272-23368-18-git-send-email-clord@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6uHk5aFkLBJXIU2eN43kXSbuokBMVSCpJ" Subject: Re: [Qemu-devel] [PATCH v3 17/32] blockdev: Separate bochs probe from its driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Colin Lord , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6uHk5aFkLBJXIU2eN43kXSbuokBMVSCpJ From: Max Reitz To: Colin Lord , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org Message-ID: Subject: Re: [PATCH v3 17/32] blockdev: Separate bochs probe from its driver References: <1467732272-23368-1-git-send-email-clord@redhat.com> <1467732272-23368-18-git-send-email-clord@redhat.com> In-Reply-To: <1467732272-23368-18-git-send-email-clord@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 05.07.2016 17:24, Colin Lord wrote: > Modifies the bochs probe to return the format name as well as the > score as the final step of separating the probe function from the > driver. This keeps the probe completely independent of the driver, > making future modularization easier to accomplish. Returning the format= > name as well as the score allows the score to be correlated to the > driver without the probe function needing to be part of the driver. >=20 > Signed-off-by: Colin Lord > --- > block.c | 19 +++++++++++++++++++ > block/bochs.c | 1 - > block/probe/bochs.c | 25 ++++++++++++++++--------- > include/block/probe.h | 3 ++- > 4 files changed, 37 insertions(+), 11 deletions(-) As I've proposed before, maybe this would be a good place to rename the probing function to bdrv_bochs_probe() since it is no longer a static function. >=20 > diff --git a/block.c b/block.c > index 88a05b2..eab8a6e 100644 > --- a/block.c > +++ b/block.c > @@ -25,6 +25,7 @@ > #include "trace.h" > #include "block/block_int.h" > #include "block/blockjob.h" > +#include "block/probe.h" > #include "qemu/error-report.h" > #include "module_block.h" > #include "qemu/module.h" > @@ -56,6 +57,13 @@ > =20 > #define NOT_DONE 0x7fffffff /* used while emulated sync operation in p= rogress */ > =20 > +typedef const char *BdrvProbeFunc(const uint8_t *buf, int buf_size, > + const char *filename, int *score); > + > +static BdrvProbeFunc *format_probes[] =3D { > + bochs_probe, > +}; Works, but... Eh. Something like the following would suit my personal tastes (I think by now everyone should have realized that I have horrible taste) better: typedef struct BdrvProbeFunc { const char *format_name; int (*probe)(const uint8_t *buf, int buf_size, const char *filename); } BdrvProbeFunc; static BdrvProbeFunc *format_probes[] =3D { { "bochs", bochs_probe }, }; It just feels strange to me that the probing function always returns a constant string. (This is an optional suggestion, you don't need to follow it.) > + > static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =3D > QTAILQ_HEAD_INITIALIZER(graph_bdrv_states); > =20 > @@ -576,6 +584,8 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int= buf_size, > const char *filename) > { > int score_max =3D 0, score; > + const char *format_max =3D NULL; > + const char *format; > size_t i; > BlockDriver *drv =3D NULL, *d; > =20 > @@ -595,6 +605,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, in= t buf_size, > } > } > =20 > + for (i =3D 0; i < ARRAY_SIZE(format_probes); i++) { > + format =3D format_probes[i](buf, buf_size, filename, &score); > + if (score > score_max) { > + score_max =3D score; > + format_max =3D format; > + drv =3D bdrv_find_format(format_max); > + } > + } I think the bdrv_find_format() call should be done after the loop (otherwise we may unnecessarily load some formats which we then actually don't use). > + > return drv; > } [...] > diff --git a/block/probe/bochs.c b/block/probe/bochs.c > index 8adc09f..8206930 100644 > --- a/block/probe/bochs.c > +++ b/block/probe/bochs.c > @@ -3,19 +3,26 @@ > #include "block/probe.h" > #include "block/driver/bochs.h" > =20 > -int bochs_probe(const uint8_t *buf, int buf_size, const char *filename= ) > +const char *bochs_probe(const uint8_t *buf, int buf_size, const char *= filename, > + int *score) > { > + const char *format =3D "bochs"; > const struct bochs_header *bochs =3D (const void *)buf; > + assert(score); > + *score =3D 0; > =20 > - if (buf_size < HEADER_SIZE) > - return 0; > + if (buf_size < HEADER_SIZE) { > + return format; > + } > =20 > if (!strcmp(bochs->magic, HEADER_MAGIC) && > - !strcmp(bochs->type, REDOLOG_TYPE) && > - !strcmp(bochs->subtype, GROWING_TYPE) && > - ((le32_to_cpu(bochs->version) =3D=3D HEADER_VERSION) || > - (le32_to_cpu(bochs->version) =3D=3D HEADER_V1))) > - return 100; > + !strcmp(bochs->type, REDOLOG_TYPE) && > + !strcmp(bochs->subtype, GROWING_TYPE) && > + ((le32_to_cpu(bochs->version) =3D=3D HEADER_VERSION) || > + (le32_to_cpu(bochs->version) =3D=3D HEADER_V1))) { > + *score =3D 100; > + return format; > + } Ah. Seems like I've complained too early. :-) Max > =20 > - return 0; > + return format; > } --6uHk5aFkLBJXIU2eN43kXSbuokBMVSCpJ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEvBAEBCAAZBQJXfScHEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrXgU B/4+R3cUfkNnRlvnpaIXVkeOjv2H2M1s5DoC2mQYCvs4ijcJ8TQZ8yQJm9S1TA5x UcCzGXYvnFk5UrUqvMfIruPmvw5EgKn46n4BTQ9ctHJJsLJ7SCzqeJuFcM13GRU7 qcL+Egk1+jgBGpS9L2eW55YmlFDIbNnoyMVNmQSr7rQdWC0c6SrJUOomGuaA25YD cFYZt3immx5DpBmTNfUbXufi5ocTmQJlA3tRX476SarPanqZpcBC3SPVPUVp7XIf TOsmQv/lwLb2w+UwV0mv+5GsnwOSsqeiz7X0TrlHz3oKnWsxyIcHZK6BnwxO8dnS Ey5fCAcGG7nhrXv4Cxy54ixx =8DUJ -----END PGP SIGNATURE----- --6uHk5aFkLBJXIU2eN43kXSbuokBMVSCpJ--