On Tue, Jun 19, 2012 at 03:55:47PM -0600, Eric Blake wrote:
> On 06/19/2012 04:26 AM, Dipankar Sarma wrote:
> >
> > src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++++++-------
> > src/conf/domain_conf.h | 2 +-
> > src/qemu/qemu_command.c | 9 +++++----
> > src/vmx/vmx.c | 28 ++++++++++++++++------------
> > src/vmx/vmx.h | 6 +++---
> > 5 files changed, 58 insertions(+), 27 deletions(-)
>
> This change is a bit bigger than the last; is there any way you can add
> to tests/qemuxml2argvtest.c (and by implication, to
> tests/qemuxml2argvdata) to cover the new code paths?
Will do.
>
> >
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>
> > @@ -3004,8 +3022,15 @@ virDomainDiskDefAssignAddress(virCapsPtr caps,
> > virDomainDiskDefPtr def)
> > switch (def->bus) {
> > case VIR_DOMAIN_DISK_BUS_SCSI:
> > def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
> > -
> > - if (caps->hasWideScsiBus) {
> > + if (STREQ(ddef->os.arch, "ppc64") &&
>
> This doesn't feel right. We shouldn't be hard-coding strings into the
> generic domain_conf code, so much as letting specific hypervisor drivers
> be making decisions. Remember, running LXC on ppc64 does not
> necessarily have the same default bus handling as running a ppc64 guest
> via qemu on an x86 machine. We either need a new field in caps (similar
> to caps->hasWideScsiBus) or even a callback function, so that the
> hypervisor driver code can answer questions about what defaults to use
> as part of the existing caps mechanism. The fact that you even had to
> touch vmx and qemu code in the same patch for a feature that you are
> really only trying to add to qemu support is further evidence that this
> is not quite right.
Thanks for the hint. That seems cleaner. It should be useful for some
other changes for ppc64 that I am working on. I will rework this patch.
Thanks
Dipankar
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
