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 <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/ > > 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
signature.asc
Description: OpenPGP digital signature
