Re: [Qemu-devel] [PATCH 1/1] Support default block interfaces per QEMUMachine

[Markus Armbruster]

Alexander Graf <address@hidden> writes:

> On 19.11.2012, at 14:44, Christian Borntraeger wrote:
>
>> On 19/11/12 14:36, Alexander Graf wrote:
>>> 
>>> On 12.11.2012, at 09:22, Christian Borntraeger wrote:
>>> 
>>>> There are QEMUMachines that have neither IF_IDE nor IF_SCSI as a
>>>> default/standard interface to their block devices / drives. Therfore,
>>>> this patch introduces a new field default_block per QEMUMachine struct.
>>>> The prior use_scsi field becomes thereby obsolete and is replaced through
>>>> .default_block = DEFAULT_SCSI.
>>>> 
>>>> Based on an initial patch from Einar Lueck <address@hidden>
>>>> 
>>>> Signed-off-by: Christian Borntraeger <address@hidden>
>>>> ---
>>>> blockdev.c          |    4 ++--
>>>> blockdev.h          |   29 ++++++++++++++++++++++++++++-
>>>> hw/boards.h         |    3 ++-
>>>> hw/device-hotplug.c |    2 +-
>>>> hw/highbank.c       |    2 +-
>>>> hw/leon3.c          |    1 -
>>>> hw/mips_jazz.c      |    4 ++--
>>>> hw/pc_sysfw.c       |    2 +-
>>>> hw/puv3.c           |    1 -
>>>> hw/realview.c       |    7 ++++---
>>>> hw/s390-virtio.c    |   16 +---------------
>>>> hw/spapr.c          |    2 +-
>>>> hw/sun4m.c          |   24 ++++++++++++------------
>>>> hw/versatilepb.c    |    4 ++--
>>>> hw/vexpress.c       |    4 ++--
>>>> hw/xilinx_zynq.c    |    2 +-
>>>> vl.c                |   20 +++++++++++---------
>>>> 17 files changed, 71 insertions(+), 56 deletions(-)
>>>> 
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index e73fd6e..aca3c14 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
>>>>    return true;
>>>> }
>>>> 
>>>> -DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>> +DriveInfo *drive_init(QemuOpts *opts, BlockDefault block_default)
>>>> {
>>>>    const char *buf;
>>>>    const char *file = NULL;
>>>> @@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int 
>>>> default_to_scsi)
>>>>            return NULL;
>>>>    }
>>>>    } else {
>>>> -        type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>> +        type = block_if_from_default(block_default);
>>>>    }
>>>> 
>>>>    max_devs = if_max_devs[type];
>>>> diff --git a/blockdev.h b/blockdev.h
>>>> index 5f27b64..aba6d77 100644
>>>> --- a/blockdev.h
>>>> +++ b/blockdev.h
>>>> @@ -24,6 +24,33 @@ typedef enum {
>>>>    IF_COUNT
>>>> } BlockInterfaceType;
>>>> 
>>>> +/* For machine default interface. */
>>>> +typedef enum {
>>>> +    DEFAULT_IDE = 0,
>>>> +    DEFAULT_SCSI,
>>>> +    DEFAULT_FLOPPY,
>>>> +    DEFAULT_PFLASH,
>>>> +    DEFAULT_MTD,
>>>> +    DEFAULT_SD,
>>>> +    DEFAULT_VIRTIO,
>>>> +    DEFAULT_XEN
>>>> +} BlockDefault;
>>> 
>>> Why a new enum? Can't we just reuse the IF_ ones?
>>> 
>>> Also, traditionally Markus has had very strong opinions on anything
>>> IF_ and -drive related. It's probably a good idea to get him in the
>>> loop :).
>>> 
>>> Alex
>> 
>> Review feedback from the first cycle. Anthony wanted to make the
>> default (field is not specified at all) a
>> sane default, which means IDE must be 0.
>
> IF_ are internal constants, we can change them to our liking. So if we
> just make IF_IDE=0, all is well, no? Worst case we can always have an
> IF_DEFAULT=0 that falls back to IF_IDE.
>
> I really dislike having the same list twice, just with different but
> almost identical names.

Generalizing QEMUMachine member use_scsi to a default BlockInterfaceType
makes plenty of sense to me.

I'm not sure "no initializer means IDE" is such a hot idea, but since
it's how use_scsi has always worked, I'm okay with making its
replacement work like that, too.

Like Alex, I dislike inventing yet another enumeration for this
purpose.  Let's use BlockInterfaceType.

In theory, we can change values of internal enumerations freely.  In
practice, such changes can run afoul of leaky abstractions, such as C89
array initializers and sloppy tests against zero.  Careful review
advised.