[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 11/13] qemu: log error when domain has an upsupported IDE controller



$SUBJ:  s/upsupported/unsupported


On 05/05/2015 02:03 PM, Laine Stump wrote:
> We have previously effectively ignored all <controller type='ide'>
> elements in a domain definition.
> 
> On the i440fx-based machinetypes there is an IDE controller that is
> included in the chipset and can't be removed (which is the ide
> controller with index='0'>), so it makes sense to ignore that one
> controller. However, if an i440fx domain has a 2nd controller, nothing
> catches this error (unless you also have a disk attached to it, in
> which case qemu will complain that you're trying to use the ide
> controller named "ide1", which doesn't exist), and if any other type
> of domain has even a single controller defined, it will be incorrectly
> ignored.
> 
> Ignoring a bogus controller definition isn't such a big problem, as
> long as an error is logged when any disk is attached to that
> non-existent controller. But in the case of q35-based machinetypes,
> the hardcoded id ("alias" in libvirt terms) of its builtin SATA
> controller is "ide", which happens to be the same id as the builtin
> IDE controller on i440fx machinetypes. So libvirt creates a
> commandline believing that it is connecting the disk to the builtin
> (but actually nonexistent) IDE controller, qemu thinks that libvirt
> wanted that disk connected to the builtin SATA controller, and
> everybody is happy.
> 
> Until you try to connect a 2nd disk to the IDE controller. Then qemu
> will complain that you're trying to set unit=1 on a controller that
> requires unit=0 (SATA controllers are organized differently than IDE
> controllers).
> 
> libvirt should really be saying what it means, and meaning what it
> says, and that's what this patch does - after this patch, if a domain
> has an IDE controller defined for a machinetype that has no IDE
> controllers, libvirt will log an error about the controller itself as
> it is building the qemu commandline (rather than a (possible) error
> from qemu about disks attached to that controller). This is done by
> rearranging the loop that creates controller command strings in
> qemuBuildCommandline() so that it also calls
> qemuBuildControllerDevStr() for IDE controllers unless it is the
> primary controller on an i440fx machine (previously it would *always*
> skip IDE controllers). Then qemuBuildControllerDevStr() is modified to
> log an appropriate error in the case of IDE controllers.
> 
> In the future, if we add support for extra IDE controllers (piix3-ide
> and/or piix4-ide) we can just add it into the IDE case in
> qemuBuildControllerDevStr(). For now, nobody seems anxious to add
> extra support for an aging and very slow controller, when there are so
> many better options available.
> 
> (note that the body of the controller loop in qemuBuildCommandline()
> was cleaned up in the process to eliminate duplicated code, but other
> than the addition of calling qemuBuildControllerDevStr() for IDE, it
> is functionally unchanged).
> ---
>  src/qemu/qemu_command.c                            | 101 ++++++++++++---------
>  .../qemuxml2argv-disk-blockio.args                 |   2 +-
>  .../qemuxml2argvdata/qemuxml2argv-disk-blockio.xml |   1 -
>  .../qemuxml2argv-disk-ide-drive-split.args         |   2 +-
>  .../qemuxml2argv-disk-ide-drive-split.xml          |   1 -
>  .../qemuxml2argv-disk-source-pool-mode.args        |   2 +-
>  .../qemuxml2argv-disk-source-pool-mode.xml         |   1 -
>  .../qemuxml2argv-disk-source-pool.args             |   2 +-
>  .../qemuxml2argv-disk-source-pool.xml              |   1 -
>  .../qemuxml2xmlout-disk-source-pool.xml            |   1 -
>  10 files changed, 63 insertions(+), 51 deletions(-)
> 


I'd say a "soft" ACK - things seem OK to me...  Rules are defined, the
commit message is very descriptive, code motion for QEMU_CAPS_ICH9_AHCI,
and enforcing IDE rules  What scares me most is the unknown of what
havoc or misconfigurations are "out there"... TBH: I close my eyes and
hope this code works ;-)...

Hopefully another set of eyes can look as well!

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]