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

[John Snow]

On 07/06/2016 04:24 AM, Kevin Wolf wrote:
> Am 05.07.2016 um 22:50 hat John Snow geschrieben:
>>
>>
>> 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/
> 
> block/bochs/ if anything. We don't have to nest deeply just because we
> can. I don't really like new subdirectories, but when all drivers start
> to have multiple files, it might be unavoidable.
> 
>> bochs.c
>> probe.c (or bochs-probe.c)
>>
>> and
>>
>> include/block/drivers/bochs/
>>
>> common.h (or internal.h)
> 
> block/bochs/internal.h (or bochs.h)
> 
> Just like we already have some header files directly in block/ (e.g.
> qcow2.h). They are internal to the block driver, so no reason to move
> them to the global include directory.
> 
> Kevin
> 

I was actually curious about this. [CCing Markus, our new #include Czar.
[or Kaiser?]]

Recently the include files in hw/ide/ were moved to include/ without
anybody mentioning it to me, including internal.h.

Why?

I don't know.

--js