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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]

On 05.07.2016 22:50, John Snow wrote:
> 
> 
> 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 <clord@redhat.com>
>>> ---
>>>  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?

Yea (or “Nay”?). I'd rather not have as many directories in block/ as we
have files there right now and in most of these directories just two
files, for two reasons:

(1) I don't want it, because of my personal taste. If you just did it, I
probably wouldn't complain for too long, though.

(2) Code motion shouldn't be done without a good reason. I know this is
of no concern to upstream (which we are talking about), but it's always
iffy when it comes to backports. And I am a Red Hat employee, so I am
paid to think about them.

Also, there's another argument: As far as I know we sooner or later want
to make probing some kind of a block driver anyway (i.e. if you choose
the "probe" block driver, it'll automatically replace itself by the
right one). So in that sense, one could actually argue that probing is a
block driver.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]