Hi Laine,
On Fri, Nov 20, 2015 at 06:55:09PM -0500, Laine Stump wrote:
> On 11/20/2015 03:20 PM, Guido Günther wrote:
> >so handle it like I440FX
> >---
> > src/qemu/qemu_command.c | 17 ++++++++++++-----
> > src/qemu/qemu_domain.c | 7 +++++++
> > src/qemu/qemu_domain.h | 1 +
> > 3 files changed, 20 insertions(+), 5 deletions(-)
> >
> >diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >index ef5ef93..d6b7f09 100644
> >--- a/src/qemu/qemu_command.c
> >+++ b/src/qemu/qemu_command.c
> >@@ -1054,11 +1054,13 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
> > */
> > return virAsprintf(&controller->info.alias, "pci.%d", controller->idx);
> > } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
> >- /* for any machine based on I440FX, the first (and currently
> >+ /* for any machine based on I440FX or G3Beige, the first (and currently
> > * only) IDE controller is an integrated controller hardcoded
> > * with id "ide"
> > */
> >- if (qemuDomainMachineIsI440FX(domainDef) && controller->idx == 0)
> >+ if ((qemuDomainMachineIsI440FX(domainDef) ||
> >+ qemuDomainMachineIsG3Beige(domainDef)) &&
> >+ controller->idx == 0)
> > return VIR_STRDUP(controller->info.alias, "ide");
> > } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) {
> > /* for any Q35 machine, the first SATA controller is the
> >@@ -4915,10 +4917,13 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
> > case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> > /* Since we currently only support the integrated IDE controller
> >- * on 440fx, if we ever get to here, it's because some other
> >+ * on 440fx and G3Beige, if we ever get to here, it's because some other
> > * machinetype had an IDE controller specified, or a 440fx had
> > * multiple ide controllers.
> > */
> >+ if (qemuDomainMachineIsG3Beige(domainDef))
> >+ break;
> >+
>
> This would cause us to ignore the fact that the config has a secondary IDE
> controller defined, and we don't support adding a 2nd IDE controller to the
> qemu commandline. I think it should instead report the following error
> message just like i440FX (unless the g3beige has an implicit secondary IDE
> and it is properly named).
Given that we can handle all machine types the same so instead having to
add machine types at three locations I added a single function to handle
this in the upcoming patch. Hope that's o.k., if not I'm happy to split
this up again.
>
> Other than that, ACK (and thanks for taking the time to look for other
> machines that have builtin IDE controllers and writing the patches! I got a
> private mail about the breakage and asked the question about which machines
> have builtint IDE on qemu-devel, but hadn't had any more time to follow up).
All three were mentioned in a Debian bug so I did not too much
research. I had a quik look at the other machine types and didn't spot
any IDE in it though.
> > if (qemuDomainMachineIsI440FX(domainDef))
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > _("Only a single IDE controller is unsupported "
>
> Ugh. I should have said "supported" there rather than "unsupported". Can you
> fix that while you're touching things?
Will do, thanks for review!
-- Guido
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list