From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKXe8-0006eN-Bz for qemu-devel@nongnu.org; Tue, 05 Jul 2016 17:12:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKXe6-0003Fu-3J for qemu-devel@nongnu.org; Tue, 05 Jul 2016 17:12:43 -0400 References: <1467732272-23368-1-git-send-email-clord@redhat.com> <1467732272-23368-5-git-send-email-clord@redhat.com> <20160705154925.GA6553@redhat.com> <32a142e7-8e46-9ae7-30b0-df42876601cb@redhat.com> From: John Snow Message-ID: <6c680fb5-9ba5-7875-470d-86c9a7e21fc2@redhat.com> Date: Tue, 5 Jul 2016 17:12:31 -0400 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , "Daniel P. Berrange" , Colin Lord Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org 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 par= t of >>>> the process of modularizing block drivers. Having the probe function= s >>>> separate from the rest of the driver allows us to probe without havi= ng >>>> to potentially unnecessarily load the driver. >>>> >>>> Signed-off-by: Colin Lord >>>> --- >>>> 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 structu= re >> 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 modula= r >> 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 metho= d. >> >> (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? >=20 > Yea (or =93Nay=94?). 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: >=20 > (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. >=20 > (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. >=20 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 wan= t > 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. >=20 Doesn't really sound like an argument against the file layout you're replying to. > Max >=20 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 v= 3. --js