[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs prob
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file |
Date: |
Thu, 7 Jul 2016 11:45:42 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 07/07/2016 02:36 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
>
>> 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 <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/
>>>
>>> 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.
>
> You're the maintainer, move them right back :)
>
> If a header is only included from one directory, and that directory
> actually has some thematic focus, then the header is probably private,
> and should probably sit in that directory.
>
> Else, the header is probably public, and should sit somewhere under
> include/.
>
> When we deviate from this rule, it's usually ugly. Example:
> include/block/block_int.h.
>
OK, thanks. Just making sure I didn't miss a memo on some more
militaristic #include paradigm.
Kevin's suggestion for organization sounds good.
--js
- [Qemu-devel] [PATCH v3 00/32] Dynamic module loading for block drivers, Colin Lord, 2016/07/05
- Re: [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Paolo Bonzini, 2016/07/07
- Re: [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, John Snow, 2016/07/07
- Re: [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Kevin Wolf, 2016/07/08
- Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Colin Lord, 2016/07/07
- Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Kevin Wolf, 2016/07/08
- Re: [Qemu-devel] [Qemu-block] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Markus Armbruster, 2016/07/08
- Re: [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Paolo Bonzini, 2016/07/07
Re: [Qemu-devel] [PATCH v3 04/32] blockdev: Move bochs probe into separate file, Max Reitz, 2016/07/06
[Qemu-devel] [PATCH v3 05/32] blockdev: Move cloop probe to its own file, Colin Lord, 2016/07/05