Re: [Qemu-block] [PATCH v3 17/32] blockdev: Separate bochs probe from its driver

[Max Reitz]

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 <address@hidden>
> ---
>  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