Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file

[John Snow]

On 07/05/2016 11:49 AM, Daniel P. Berrange wrote:
> On Tue, Jul 05, 2016 at 11:24:04AM -0400, Colin Lord wrote:
>> This puts the bochs probe function into its own separate file as part of
>> the process of modularizing block drivers. Having the probe functions
>> separate from the rest of the driver allows us to probe without having
>> to potentially unnecessarily load the driver.
>>
>> Signed-off-by: Colin Lord <address@hidden>
>> ---
>>  block/Makefile.objs          |  1 +
>>  block/bochs.c                | 55 
>> ++------------------------------------------
>>  block/probe/bochs.c          | 21 +++++++++++++++++
> 
> Do we really need a sub-dir for this ?  If we were going to
> have sub-dirs under block/, I'd suggest we have one subdir
> per block driver, not spread code for one block driver
> across multiple dirs.
> 

Admittedly I have been nudging Colin to shoot from the hip a bit on
filename organization because I was short of ideas.

Some ideas:

(1) A combined probe.c file. This keeps the existing organization and
localizes everything to just one new file.

Downside: many formats rely on at least some minimal amount of structure
and constant definitions, and some of those overlap with each other.
qcow and qcow2 both using "QcowHeader" would be a prominent example.

They could all be disentangled, but it's less clear on where all the
common definitions go. A common probe.h is a bad idea since the modular
portion of the driver has no business gaining access to other formats'
definitions. We could create a probe.c and matching
include/block/bdrv/fmt.h includes, but we lost our zeal for this method.

(2) Separate probe files for each driver.

What we went with. Keeps refactoring to a minimum. Adds a bunch of
little files, but it's minimal and fairly noninvasive.

Like #1 though, we still have to figure out what to do with the common
includes.

> IMHO a block/bochs-probe.c would be better, unless we did
> move block/bochs.c into a block/bochs/driver.c dir.
> 
> Either way, you should update MAINTAINERS file to record
> this newly added filename, against the bochs entry. The
> same applies to most other patches in this series adding
> new files.
> 
> 
> Regards,
> Daniel
> 

So, something like:

block/drivers/bochs/

bochs.c
probe.c (or bochs-probe.c)

and

include/block/drivers/bochs/

common.h (or internal.h)


Any objections from the gallery?

--js