On 05/14/2015 12:25 PM, Laine Stump wrote: > On 05/08/2015 09:44 PM, John Ferlan wrote: >> On 05/05/2015 02:03 PM, Laine Stump wrote: >>> libvirt enforces bus (channel to qemu) == 0 for those qemu binaries >>> that don't support the channel argument to scsi disk devices, but >>> still adds "channel=0" in those cases. Apparently nobody with a qemu >>> old enough to not support channel ever uses these devices, because >>> they otherwise should get an error. >>> >>> This patch only adds channel when the scsi-disk.channel capability is >>> set for the qemu binary. Most of the change in the patch is updating >>> patches to either 1) remove channel=0 from the .args file of the test, >>> or 2) reposition channel=0 to precede the bus arg (because this makes >>> the code cleaner/simpler) and change the DO_TEST() invocation for the >>> test to add QEMU_CAPS_SCSI_DISK_CHANNEL. >>> >>> I'm uncommitted about whether or not it is worthwhile to push this >>> patch. On one hand, I'm fairly certain this is what is correct; on the >>> other hand nobody has ever complained about it, and at this point >>> almost everyone is using new enough qemu that it supports channel. >>> --- >>> src/qemu/qemu_command.c | 7 +++--- >>> .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +- >>> .../qemuxml2argv-disk-scsi-disk-split.args | 8 +++---- >>> .../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +- >>> .../qemuxml2argv-disk-scsi-disk-wwn.args | 4 ++-- >>> .../qemuxml2argv-disk-scsi-lun-passthrough.args | 4 ++-- >>> .../qemuxml2argv-disk-scsi-megasas.args | 2 +- >>> .../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +- >>> .../qemuxml2argv-disk-scsi-vscsi.args | 2 +- >>> .../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +- >>> .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +- >>> .../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +- >>> .../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +- >>> .../qemuxml2argv-pseries-vio-user-assigned.args | 2 +- >>> .../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +- >>> tests/qemuxml2argvtest.c | 25 +++++++++++++--------- >>> 16 files changed, 38 insertions(+), 32 deletions(-) >>> >> I'm in agreement with you vis-a-vis whether it's worthwhile. Nothing's >> broken, so the axiom may apply. However, we don't know if it's broken >> "under the covers" since we don't know the affect of adding channel=# to >> a qemu that doesn't support it since we don't seem to have one of those >> today. >> >> OTOH since you added the QEMU_CAPS_SCSI_DISK_CHANNEL bit in and that >> alone didn't break anything, then it would appear this change should be >> OK. Other than changing the order/placement of channel (which seems to >> be a non-issue), it seems these changes are fine. >> >> Should I assume you've started guests with all these changes? And >> perhaps restart libvirtd to make sure noone disappears? > I did restart libvirtd with a running guest with SCSI disk, no problems, > and then destroyed and restart the guest (so it uses the different > argument ordering), no problems. I don't have a reasonable way to test > the case when channel= isn't supported, since that has even been > backported to RHEL6. > >> quasi-ACK, your call depending on your comfort level. > I'm planning on pushing this later today, if there are no objections. Along with RHEL6/CentOS6 supporting channel, RHEL5 is old enough that it doesn't even support -device, so I have nothing of the proper vintage to test for non-support of channel, and this makes me doubt that anybody anywhere has a system that would be affected by this patch, so I'm dropping it too. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list