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. > > Signed-off-by: Colin Lord <[email protected]> > --- > 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.
>
> 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 @@
>
> #define NOT_DONE 0x7fffffff /* used while emulated sync operation in
> progress */
>
> +typedef const char *BdrvProbeFunc(const uint8_t *buf, int buf_size,
> + const char *filename, int *score);
> +
> +static BdrvProbeFunc *format_probes[] = {
> + 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[] = {
{ "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 =
> QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
>
> @@ -576,6 +584,8 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int
> buf_size,
> const char *filename)
> {
> int score_max = 0, score;
> + const char *format_max = NULL;
> + const char *format;
> size_t i;
> BlockDriver *drv = NULL, *d;
>
> @@ -595,6 +605,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int
> buf_size,
> }
> }
>
> + for (i = 0; i < ARRAY_SIZE(format_probes); i++) {
> + format = format_probes[i](buf, buf_size, filename, &score);
> + if (score > score_max) {
> + score_max = score;
> + format_max = format;
> + drv = 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"
>
> -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 = "bochs";
> const struct bochs_header *bochs = (const void *)buf;
> + assert(score);
> + *score = 0;
>
> - if (buf_size < HEADER_SIZE)
> - return 0;
> + if (buf_size < HEADER_SIZE) {
> + return format;
> + }
>
> if (!strcmp(bochs->magic, HEADER_MAGIC) &&
> - !strcmp(bochs->type, REDOLOG_TYPE) &&
> - !strcmp(bochs->subtype, GROWING_TYPE) &&
> - ((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
> - (le32_to_cpu(bochs->version) == HEADER_V1)))
> - return 100;
> + !strcmp(bochs->type, REDOLOG_TYPE) &&
> + !strcmp(bochs->subtype, GROWING_TYPE) &&
> + ((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
> + (le32_to_cpu(bochs->version) == HEADER_V1))) {
> + *score = 100;
> + return format;
> + }
Ah. Seems like I've complained too early. :-)
Max
>
> - return 0;
> + return format;
> }
signature.asc
Description: OpenPGP digital signature
