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

[Markus Armbruster]

John Snow <address@hidden> writes:

> 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.

You're the maintainer, move them right back :)

If a header is only included from one directory, and that directory
actually has some thematic focus, then the header is probably private,
and should probably sit in that directory.

Else, the header is probably public, and should sit somewhere under
include/.

When we deviate from this rule, it's usually ugly.  Example:
include/block/block_int.h.