On 05.07.2016 23:12, John Snow wrote: > > > On 07/05/2016 05:00 PM, Max Reitz wrote: >> 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. >> > > Reason: We haven't had modules before. Now we do. Shared constants and > structures need to go somewhere, probes need to get split out. > > Now, existing files (that will become the modular portions) can stay put > if you'd like, but the probes and common includes need to go somewhere. > > Block drivers will be more decentralized than they've ever been. 1-3 > files per each driver, depending on if they have a probe or if they have > shared definitions that the probe needs to access. > > This at least raises the question for organization to minimize future > confusion. The answer to that question might be "Please leave the core > modules/drivers alone," but the question gets asked. > >> 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. >> > > Doesn't really sound like an argument against the file layout you're > replying to. > >> Max >> > > 12 weeks isn't a very long time, so if you have a preferred > organizational structure, I'd prefer you present that instead of just a > NACK, or put your vote for the currently presented organization in this v3.
I liked block/probe/. Max
signature.asc
Description: OpenPGP digital signature
