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 <cl...@redhat.com> > --- > 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