On 07/07/2016 02:36 AM, Markus Armbruster wrote: > John Snow <js...@redhat.com> 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 <cl...@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/ >>> >>> 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. >
OK, thanks. Just making sure I didn't miss a memo on some more militaristic #include paradigm. Kevin's suggestion for organization sounds good. --js
