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