From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56497) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKXIL-00078z-VZ for qemu-devel@nongnu.org; Tue, 05 Jul 2016 16:50:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKXIK-0001WG-0J for qemu-devel@nongnu.org; Tue, 05 Jul 2016 16:50:12 -0400 References: <1467732272-23368-1-git-send-email-clord@redhat.com> <1467732272-23368-5-git-send-email-clord@redhat.com> <20160705154925.GA6553@redhat.com> From: John Snow Message-ID: <32a142e7-8e46-9ae7-30b0-df42876601cb@redhat.com> Date: Tue, 5 Jul 2016 16:50:00 -0400 MIME-Version: 1.0 In-Reply-To: <20160705154925.GA6553@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: "Daniel P. Berrange" , Colin Lord Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com 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 >> --- >> 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? --js