[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs prob
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file |
Date: |
Wed, 6 Jul 2016 14:39:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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 <address@hidden>
>>>>> ---
>>>>> 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
- Re: [Qemu-block] [PATCH v3 06/32] blockdev: Move luks probe to its own file, (continued)
- [Qemu-block] [PATCH v3 07/32] blockdev: Move dmg probe to its own file, Colin Lord, 2016/07/05
- [Qemu-block] [PATCH v3 08/32] blockdev: Move parallels probe to its own file, Colin Lord, 2016/07/05
- [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Colin Lord, 2016/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Kevin Wolf, 2016/07/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, John Snow, 2016/07/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Markus Armbruster, 2016/07/07
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, John Snow, 2016/07/07
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Paolo Bonzini, 2016/07/07
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, John Snow, 2016/07/07
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Kevin Wolf, 2016/07/08
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Colin Lord, 2016/07/07
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Kevin Wolf, 2016/07/08
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Markus Armbruster, 2016/07/08