From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50377) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKpOH-0000WG-2e for qemu-devel@nongnu.org; Wed, 06 Jul 2016 12:09:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKpOE-00063u-8h for qemu-devel@nongnu.org; Wed, 06 Jul 2016 12:09:32 -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> <20160706082428.GD5233@noname.str.redhat.com> From: John Snow Message-ID: <998cb3c6-46c9-e60c-d807-102911912384@redhat.com> Date: Wed, 6 Jul 2016 12:09:17 -0400 MIME-Version: 1.0 In-Reply-To: <20160706082428.GD5233@noname.str.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: Kevin Wolf Cc: "Daniel P. Berrange" , Colin Lord , qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com, Markus Armbruster 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 >>>> --- >>>> 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