$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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list