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