From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35342) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKi8O-0005Y4-6G for qemu-devel@nongnu.org; Wed, 06 Jul 2016 04:24:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKi8M-00058B-3b for qemu-devel@nongnu.org; Wed, 06 Jul 2016 04:24:39 -0400 Date: Wed, 6 Jul 2016 10:24:28 +0200 From: Kevin Wolf Message-ID: <20160706082428.GD5233@noname.str.redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <32a142e7-8e46-9ae7-30b0-df42876601cb@redhat.com> 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: John Snow Cc: "Daniel P. Berrange" , Colin Lord , qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com 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