diff mbox

[v3,04/32] blockdev: Move bochs probe into separate file

Message ID 1467732272-23368-5-git-send-email-clord@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

clord@redhat.com July 5, 2016, 3:24 p.m. UTC
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 <clord@redhat.com>
---
 block/Makefile.objs          |  1 +
 block/bochs.c                | 55 ++------------------------------------------
 block/probe/bochs.c          | 21 +++++++++++++++++
 include/block/driver/bochs.h | 40 ++++++++++++++++++++++++++++++++
 include/block/probe.h        |  6 +++++
 5 files changed, 70 insertions(+), 53 deletions(-)
 create mode 100644 block/probe/bochs.c
 create mode 100644 include/block/driver/bochs.h
 create mode 100644 include/block/probe.h

Comments

Daniel P. Berrangé July 5, 2016, 3:49 p.m. UTC | #1
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 <clord@redhat.com>
> ---
>  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.

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
John Snow July 5, 2016, 8:50 p.m. UTC | #2
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 <clord@redhat.com>
>> ---
>>  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
Max Reitz July 5, 2016, 9 p.m. UTC | #3
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 <clord@redhat.com>
>>> ---
>>>  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.

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.

Max
John Snow July 5, 2016, 9:12 p.m. UTC | #4
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 <clord@redhat.com>
>>>> ---
>>>>  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.

--js
Kevin Wolf July 6, 2016, 8:24 a.m. UTC | #5
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 <clord@redhat.com>
> >> ---
> >>  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
Max Reitz July 6, 2016, 12:39 p.m. UTC | #6
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 <clord@redhat.com>
>>>>> ---
>>>>>  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
Max Reitz July 6, 2016, 2:19 p.m. UTC | #7
On 05.07.2016 17:24, 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 <clord@redhat.com>
> ---
>  block/Makefile.objs          |  1 +
>  block/bochs.c                | 55 ++------------------------------------------
>  block/probe/bochs.c          | 21 +++++++++++++++++
>  include/block/driver/bochs.h | 40 ++++++++++++++++++++++++++++++++
>  include/block/probe.h        |  6 +++++
>  5 files changed, 70 insertions(+), 53 deletions(-)
>  create mode 100644 block/probe/bochs.c
>  create mode 100644 include/block/driver/bochs.h
>  create mode 100644 include/block/probe.h
> 

[...]

> diff --git a/block/probe/bochs.c b/block/probe/bochs.c
> new file mode 100644
> index 0000000..8adc09f
> --- /dev/null
> +++ b/block/probe/bochs.c
> @@ -0,0 +1,21 @@
> +#include "qemu/osdep.h"
> +#include "block/block_int.h"
> +#include "block/probe.h"
> +#include "block/driver/bochs.h"
> +
> +int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
> +{
> +    const struct bochs_header *bochs = (const void *)buf;
> +
> +    if (buf_size < HEADER_SIZE)
> +	return 0;
> +
> +    if (!strcmp(bochs->magic, HEADER_MAGIC) &&
> +	!strcmp(bochs->type, REDOLOG_TYPE) &&
> +	!strcmp(bochs->subtype, GROWING_TYPE) &&
> +	((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
> +	(le32_to_cpu(bochs->version) == HEADER_V1)))
> +	return 100;
> +
> +    return 0;
> +}

Not sure what to do about the coding style here. Some people prefer code
movement to be pure, but I feel bad about doing code movement and then
leaving the code wrongly formatted. That is, in my opinion, every patch
should pass checkpatch.pl (but I won't object to a patch that doesn't
pass checkpatch.pl simply because of pre-existing code).

> diff --git a/include/block/driver/bochs.h b/include/block/driver/bochs.h
> new file mode 100644
> index 0000000..cd87256
> --- /dev/null
> +++ b/include/block/driver/bochs.h

Kevin's point that we maybe should just put this into block/ itself
(just like block/qcow2.h) is not a bad one, but I'm fine either way.

> @@ -0,0 +1,40 @@
> +#ifndef BOCHS_H
> +#define BOCHS_H

Maybe BLOCK_BOCHS_H would be better, considering that Bochs is primarily
a system emulator and its image format doesn't really have an own name.

(Compare block/qcow2.h, which uses BLOCK_QCOW2_H.)

Independently of what you decide to do in any of these three places:

Reviewed-by: Max Reitz <mreitz@redhat.com>
Max Reitz July 6, 2016, 3:41 p.m. UTC | #8
On 06.07.2016 16:19, Max Reitz wrote:
> On 05.07.2016 17:24, 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 <clord@redhat.com>
>> ---
>>  block/Makefile.objs          |  1 +
>>  block/bochs.c                | 55 ++------------------------------------------
>>  block/probe/bochs.c          | 21 +++++++++++++++++
>>  include/block/driver/bochs.h | 40 ++++++++++++++++++++++++++++++++
>>  include/block/probe.h        |  6 +++++
>>  5 files changed, 70 insertions(+), 53 deletions(-)
>>  create mode 100644 block/probe/bochs.c
>>  create mode 100644 include/block/driver/bochs.h
>>  create mode 100644 include/block/probe.h
>>
> 
> [...]
> 
>> diff --git a/block/probe/bochs.c b/block/probe/bochs.c
>> new file mode 100644
>> index 0000000..8adc09f
>> --- /dev/null
>> +++ b/block/probe/bochs.c
>> @@ -0,0 +1,21 @@
>> +#include "qemu/osdep.h"
>> +#include "block/block_int.h"
>> +#include "block/probe.h"
>> +#include "block/driver/bochs.h"
>> +
>> +int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
>> +{
>> +    const struct bochs_header *bochs = (const void *)buf;
>> +
>> +    if (buf_size < HEADER_SIZE)
>> +	return 0;
>> +
>> +    if (!strcmp(bochs->magic, HEADER_MAGIC) &&
>> +	!strcmp(bochs->type, REDOLOG_TYPE) &&
>> +	!strcmp(bochs->subtype, GROWING_TYPE) &&
>> +	((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
>> +	(le32_to_cpu(bochs->version) == HEADER_V1)))
>> +	return 100;
>> +
>> +    return 0;
>> +}
> 
> Not sure what to do about the coding style here. Some people prefer code
> movement to be pure, but I feel bad about doing code movement and then
> leaving the code wrongly formatted. That is, in my opinion, every patch
> should pass checkpatch.pl (but I won't object to a patch that doesn't
> pass checkpatch.pl simply because of pre-existing code).

I've just seen that you indeed fix this later in this series. That's
good enough for me.

Max
John Snow July 6, 2016, 4:09 p.m. UTC | #9
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 <clord@redhat.com>
>>>> ---
>>>>  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
Markus Armbruster July 7, 2016, 6:36 a.m. UTC | #10
John Snow <jsnow@redhat.com> 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 <clord@redhat.com>
>>>>> ---
>>>>>  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.
John Snow July 7, 2016, 3:45 p.m. UTC | #11
On 07/07/2016 02:36 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> 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 <clord@redhat.com>
>>>>>> ---
>>>>>>  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
Paolo Bonzini July 7, 2016, 3:59 p.m. UTC | #12
On 05/07/2016 22:50, John Snow wrote:
> 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?

I guess I'm missing why it is useful to modularize drivers that don't
need any external libraries; those that do in general are protocols and
need no probe function.

Sorry if I haven't noticed this in a previous discussion.

Paolo
Paolo Bonzini July 7, 2016, 4:01 p.m. UTC | #13
On 06/07/2016 18:09, John Snow wrote:
> Recently the include files in hw/ide/ were moved to include/ without
> anybody mentioning it to me, including internal.h.
> 
> Why?

Because hw/ide/internal.h is not so internal.  In particular, it is
included by hw/ide/pci.h, which is included by hw/i386/pc_q35.c.

Paolo
John Snow July 7, 2016, 4:14 p.m. UTC | #14
On 07/07/2016 12:01 PM, Paolo Bonzini wrote:
> 
> 
> On 06/07/2016 18:09, John Snow wrote:
>> Recently the include files in hw/ide/ were moved to include/ without
>> anybody mentioning it to me, including internal.h.
>>
>> Why?
> 
> Because hw/ide/internal.h is not so internal.  In particular, it is
> included by hw/ide/pci.h, which is included by hw/i386/pc_q35.c.
> 
> Paolo
> 

Joy. Something to fix.
clord@redhat.com July 7, 2016, 8:32 p.m. UTC | #15
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 <clord@redhat.com>
>>>> ---
>>>>  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
> 
So it sounds like what I should do is that for each driver I remove a
probe from, I'll create a new directory underneath block/ and put the
driver file, the probe file, and any new headers in it. eg for bochs,
there will be a new directory block/bochs/ under which there will be
block/bochs/bochs.c, block/bochs/bochs.h, and block/bochs/probe.c. Thus
no new headers will end up going in the include/ directory.

Also if that's what I'm doing, I assume I should leave alone any headers
already in the block/ directory that are used by other files that I'm
not touching? eg block/qed.h already exists and is used by files other
than the driver and probe, so it seems like I should leave that alone.

Colin
Markus Armbruster July 8, 2016, 7:17 a.m. UTC | #16
Kevin Wolf <kwolf@redhat.com> writes:

> 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 <clord@redhat.com>
>> >> ---
>> >>  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.

A block driver consisting of a dozen files would probably be easier to
work with in its own directory.  But three files?

Our more complex block drivers already consist of multiple files.  Have
we had problems with them all sitting in block/?  If not, why move
stuff?

>> 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)

bochs.h, please.  I don't fancy having a dozen headers called
"internal.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.

Genau.
Kevin Wolf July 8, 2016, 9:31 a.m. UTC | #17
Am 07.07.2016 um 18:01 hat Paolo Bonzini geschrieben:
> On 06/07/2016 18:09, John Snow wrote:
> > Recently the include files in hw/ide/ were moved to include/ without
> > anybody mentioning it to me, including internal.h.
> > 
> > Why?
> 
> Because hw/ide/internal.h is not so internal.  In particular, it is
> included by hw/ide/pci.h, which is included by hw/i386/pc_q35.c.

Probably it should be split in a truly internal part that remains in
hw/ide/ and part for the public interface.

Kevin
Kevin Wolf July 8, 2016, 9:37 a.m. UTC | #18
Am 07.07.2016 um 22:32 hat Colin Lord geschrieben:
> On 07/06/2016 04:24 AM, Kevin Wolf wrote:
> > Am 05.07.2016 um 22:50 hat John Snow geschrieben:
> >> 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
> > 
> So it sounds like what I should do is that for each driver I remove a
> probe from, I'll create a new directory underneath block/ and put the
> driver file, the probe file, and any new headers in it. eg for bochs,
> there will be a new directory block/bochs/ under which there will be
> block/bochs/bochs.c, block/bochs/bochs.h, and block/bochs/probe.c. Thus
> no new headers will end up going in the include/ directory.

Yes, either that or like Markus says, just create a block/bochs-probe.c
and block/bochs.h and leave the existing files where they are. Maybe
give us a bit more time to come to a conclusion. I could live with
either.

> Also if that's what I'm doing, I assume I should leave alone any headers
> already in the block/ directory that are used by other files that I'm
> not touching? eg block/qed.h already exists and is used by files other
> than the driver and probe, so it seems like I should leave that alone.

block/qed.h isn't used outside the driver. It's just that the driver
consists of multiple files. If you decide to move files, of course you
shouldn't only move qed.c, but qed-*.c, too.

Kevin
diff mbox

Patch

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 44a5416..bc0c2aa 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -24,6 +24,7 @@  block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 
 block-obj-y += crypto.o
+block-obj-y += probe/bochs.o
 
 common-obj-y += stream.o
 common-obj-y += commit.o
diff --git a/block/bochs.c b/block/bochs.c
index 6c8d0f3..11da0fd 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -28,45 +28,11 @@ 
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qemu/bswap.h"
+#include "block/driver/bochs.h"
+#include "block/probe.h"
 
 /**************************************************************/
 
-#define HEADER_MAGIC "Bochs Virtual HD Image"
-#define HEADER_VERSION 0x00020000
-#define HEADER_V1 0x00010000
-#define HEADER_SIZE 512
-
-#define REDOLOG_TYPE "Redolog"
-#define GROWING_TYPE "Growing"
-
-// not allocated: 0xffffffff
-
-// always little-endian
-struct bochs_header {
-    char magic[32];     /* "Bochs Virtual HD Image" */
-    char type[16];      /* "Redolog" */
-    char subtype[16];   /* "Undoable" / "Volatile" / "Growing" */
-    uint32_t version;
-    uint32_t header;    /* size of header */
-
-    uint32_t catalog;   /* num of entries */
-    uint32_t bitmap;    /* bitmap size */
-    uint32_t extent;    /* extent size */
-
-    union {
-        struct {
-            uint32_t reserved;  /* for ??? */
-            uint64_t disk;      /* disk size */
-            char padding[HEADER_SIZE - 64 - 20 - 12];
-        } QEMU_PACKED redolog;
-        struct {
-            uint64_t disk;      /* disk size */
-            char padding[HEADER_SIZE - 64 - 20 - 8];
-        } QEMU_PACKED redolog_v1;
-        char padding[HEADER_SIZE - 64 - 20];
-    } extra;
-} QEMU_PACKED;
-
 typedef struct BDRVBochsState {
     CoMutex lock;
     uint32_t *catalog_bitmap;
@@ -79,23 +45,6 @@  typedef struct BDRVBochsState {
     uint32_t extent_size;
 } BDRVBochsState;
 
-static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
-{
-    const struct bochs_header *bochs = (const void *)buf;
-
-    if (buf_size < HEADER_SIZE)
-	return 0;
-
-    if (!strcmp(bochs->magic, HEADER_MAGIC) &&
-	!strcmp(bochs->type, REDOLOG_TYPE) &&
-	!strcmp(bochs->subtype, GROWING_TYPE) &&
-	((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
-	(le32_to_cpu(bochs->version) == HEADER_V1)))
-	return 100;
-
-    return 0;
-}
-
 static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
diff --git a/block/probe/bochs.c b/block/probe/bochs.c
new file mode 100644
index 0000000..8adc09f
--- /dev/null
+++ b/block/probe/bochs.c
@@ -0,0 +1,21 @@ 
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+#include "block/probe.h"
+#include "block/driver/bochs.h"
+
+int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
+{
+    const struct bochs_header *bochs = (const void *)buf;
+
+    if (buf_size < HEADER_SIZE)
+	return 0;
+
+    if (!strcmp(bochs->magic, HEADER_MAGIC) &&
+	!strcmp(bochs->type, REDOLOG_TYPE) &&
+	!strcmp(bochs->subtype, GROWING_TYPE) &&
+	((le32_to_cpu(bochs->version) == HEADER_VERSION) ||
+	(le32_to_cpu(bochs->version) == HEADER_V1)))
+	return 100;
+
+    return 0;
+}
diff --git a/include/block/driver/bochs.h b/include/block/driver/bochs.h
new file mode 100644
index 0000000..cd87256
--- /dev/null
+++ b/include/block/driver/bochs.h
@@ -0,0 +1,40 @@ 
+#ifndef BOCHS_H
+#define BOCHS_H
+
+#define HEADER_MAGIC "Bochs Virtual HD Image"
+#define HEADER_VERSION 0x00020000
+#define HEADER_V1 0x00010000
+#define HEADER_SIZE 512
+
+#define REDOLOG_TYPE "Redolog"
+#define GROWING_TYPE "Growing"
+
+// not allocated: 0xffffffff
+
+// always little-endian
+struct bochs_header {
+    char magic[32];     /* "Bochs Virtual HD Image" */
+    char type[16];      /* "Redolog" */
+    char subtype[16];   /* "Undoable" / "Volatile" / "Growing" */
+    uint32_t version;
+    uint32_t header;    /* size of header */
+
+    uint32_t catalog;   /* num of entries */
+    uint32_t bitmap;    /* bitmap size */
+    uint32_t extent;    /* extent size */
+
+    union {
+        struct {
+            uint32_t reserved;  /* for ??? */
+            uint64_t disk;      /* disk size */
+            char padding[HEADER_SIZE - 64 - 20 - 12];
+        } QEMU_PACKED redolog;
+        struct {
+            uint64_t disk;      /* disk size */
+            char padding[HEADER_SIZE - 64 - 20 - 8];
+        } QEMU_PACKED redolog_v1;
+        char padding[HEADER_SIZE - 64 - 20];
+    } extra;
+} QEMU_PACKED;
+
+#endif
diff --git a/include/block/probe.h b/include/block/probe.h
new file mode 100644
index 0000000..6450ca1
--- /dev/null
+++ b/include/block/probe.h
@@ -0,0 +1,6 @@ 
+#ifndef PROBE_H
+#define PROBE_H
+
+int bochs_probe(const uint8_t *buf, int buf_size, const char *filename);
+
+#endif