On Tue, May 05, 2015 at 02:03:16PM -0400, 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.
The error was just mentioned a few lines above, there's no need to
mention it twice in the commit message.
> 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
Dropping this sentence would make the commit message shorter.
> - 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).
The cleanup should be in a separate patch to make review possible.
A shorter commit message would be helpful as well.
Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list