Re: [libvirt] [PATCH v3] qemu: Split scsi-disk into into scsi-hd and scsi-cd

On Tue, Apr 17, 2012 at 05:11:31PM +0800, Osier Yang wrote: > A "scsi-disk" device can be either a hard disk or a CD-ROM, > if there is ",media=cdrom" specified for the backend, it's > a CD-ROM, otherwise it's a hard disk. > > But upstream qemu splitted "scsi-disk" into "scsi-hd" and > "scsi-cd" since commit b443ae, and ",media=cdrom" is not > required for scsi-cd anymore. "scsi-disk" is still supported > for backwards compatibility, but no doubt we should go > foward. > > --- > No changes between v1 and v3, just rebasing. v2 missed > the test XMLs. > --- > src/qemu/qemu_capabilities.c | 3 + > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 39 ++++++++++++++---- > tests/qemuhelptest.c | 6 ++- > .../qemuxml2argv-disk-scsi-disk-split.args | 16 +++++++ > .../qemuxml2argv-disk-scsi-disk-split.xml | 44 ++++++++++++++++++++ > tests/qemuxml2argvtest.c | 3 + > 7 files changed, 102 insertions(+), 10 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index fd4ca4d..4e22f3e8 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -159,6 +159,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, > > "block-job-sync", /* 90 */ > "block-job-async", > + "scsi-cd", > ); > > struct qemu_feature_flags { > @@ -1451,6 +1452,8 @@ qemuCapsParseDeviceStr(const char *str, virBitmapPtr flags) > qemuCapsSet(flags, QEMU_CAPS_SCSI_DISK_CHANNEL); > if (strstr(str, "scsi-block")) > qemuCapsSet(flags, QEMU_CAPS_SCSI_BLOCK); > + if (strstr(str, "scsi-cd")) > + qemuCapsSet(flags, QEMU_CAPS_SCSI_CD); > > return 0; > } > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index 6550d59..2aba502 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -126,6 +126,7 @@ enum qemuCapsFlags { > QEMU_CAPS_TRANSACTION = 89, /* transaction monitor command */ > QEMU_CAPS_BLOCKJOB_SYNC = 90, /* RHEL 6.2 block_job_cancel */ > QEMU_CAPS_BLOCKJOB_ASYNC = 91, /* qemu 1.1 block-job-cancel */ > + QEMU_CAPS_SCSI_CD = 92, /* -device scsi-cd */ > > QEMU_CAPS_LAST, /* this must always be the last item */ > }; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index c82f5bc..ff82b26 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1978,8 +1978,14 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > else > virBufferAsprintf(&opt, "if=%s", bus); > > - if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) > - virBufferAddLit(&opt, ",media=cdrom"); > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { > + if ((disk->bus == VIR_DOMAIN_DISK_BUS_SCSI)) { > + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) > + virBufferAddLit(&opt, ",media=cdrom"); > + } else { > + virBufferAddLit(&opt, ",media=cdrom"); > + } > + } > > if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { > virBufferAsprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias); > @@ -2229,10 +2235,19 @@ qemuBuildDriveDevStr(virDomainDefPtr def, > goto error; > } > > - if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { > virBufferAddLit(&opt, "scsi-block"); > - else > - virBufferAddLit(&opt, "scsi-disk"); > + } else { > + if (qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) { > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) > + virBufferAddLit(&opt, "scsi-cd"); > + else > + virBufferAddLit(&opt, "scsi-hd"); > + } else { > + virBufferAddLit(&opt, "scsi-disk"); > + } > + } > + > virBufferAsprintf(&opt, ",bus=scsi%d.%d,scsi-id=%d", > disk->info.addr.drive.controller, > disk->info.addr.drive.bus, > @@ -2255,10 +2270,18 @@ qemuBuildDriveDevStr(virDomainDefPtr def, > } > } > > - if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) > - virBufferAddLit(&opt, "scsi-disk"); > - else > + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) { > + if (qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) { > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) > + virBufferAddLit(&opt, "scsi-cd"); > + else > + virBufferAddLit(&opt, "scsi-hd"); > + } else { > + virBufferAddLit(&opt, "scsi-disk"); > + } > + } else { > virBufferAddLit(&opt, "scsi-block"); > + } > > virBufferAsprintf(&opt, ",bus=scsi%d.0,channel=%d,scsi-id=%d,lun=%d", > disk->info.addr.drive.controller, > diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c > index 05fdc4d..a01c389 100644 > --- a/tests/qemuhelptest.c > +++ b/tests/qemuhelptest.c > @@ -605,7 +605,8 @@ mymain(void) > QEMU_CAPS_VIRTIO_BLK_SCSI, > QEMU_CAPS_VIRTIO_BLK_SG_IO, > QEMU_CAPS_DRIVE_COPY_ON_READ, > - QEMU_CAPS_CPU_HOST); > + QEMU_CAPS_CPU_HOST, > + QEMU_CAPS_SCSI_CD); > DO_TEST("qemu-1.0", 1000000, 0, 0, > QEMU_CAPS_VNC_COLON, > QEMU_CAPS_NO_REBOOT, > @@ -674,7 +675,8 @@ mymain(void) > QEMU_CAPS_VIRTIO_BLK_SG_IO, > QEMU_CAPS_CPU_HOST, > QEMU_CAPS_FSDEV_WRITEOUT, > - QEMU_CAPS_SCSI_BLOCK); > + QEMU_CAPS_SCSI_BLOCK, > + QEMU_CAPS_SCSI_CD); > > return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; > } > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args > new file mode 100644 > index 0000000..a79299a > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args > @@ -0,0 +1,16 @@ > +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ > +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ > +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ > +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ > +-device lsi,id=scsi1,bus=pci.0,addr=0x4 \ > +-device lsi,id=scsi2,bus=pci.0,addr=0x5 \ > +-device virtio-scsi-pci,id=scsi3,bus=pci.0,addr=0x6 \ > +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-1-0 \ > +-device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,id=scsi0-0-1-0 \ > +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-0-0 \ > +-device scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ > +-drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-0-1 \ > +-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \ > +-drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-1-1 \ > +-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=1,lun=1,drive=drive-scsi0-0-1-1,id=scsi0-0-1-1 \ > +-usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml > new file mode 100644 > index 0000000..02181fd > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml > @@ -0,0 +1,44 @@ > +<domain type='qemu'> > +<name>QEMUGuest1</name> > +<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > +<memory unit='KiB'>219136</memory> > +<currentMemory unit='KiB'>219136</currentMemory> > +<vcpu>1</vcpu> > +<os> > +<type arch='i686' machine='pc'>hvm</type> > +<boot dev='hd'/> > +</os> > +<clock offset='utc'/> > +<on_poweroff>destroy</on_poweroff> > +<on_reboot>restart</on_reboot> > +<on_crash>destroy</on_crash> > +<devices> > +<emulator>/usr/bin/qemu</emulator> > +<disk type='block' device='cdrom'> > +<source dev='/dev/HostVG/QEMUGuest1'/> > +<target dev='sda' bus='scsi'/> > +<address type='drive' controller='0' bus='0' target='1' unit='0'/> > +</disk> > +<disk type='block' device='cdrom'> > +<source dev='/dev/HostVG/QEMUGuest2'/> > +<target dev='sda' bus='scsi'/> > +<address type='drive' controller='0' bus='0' target='0' unit='0'/> > +</disk> > +<disk type='file' device='disk'> > +<source file='/tmp/scsidisk.img'/> > +<target dev='sdc' bus='scsi'/> > +<address type='drive' controller='0' bus='0' target='0' unit='1'/> > +</disk> > +<disk type='file' device='disk'> > +<source file='/tmp/scsidisk.img'/> > +<target dev='sdd' bus='scsi'/> > +<address type='drive' controller='0' bus='0' target='1' unit='1'/> > +</disk> > +<controller type='usb' index='0'/> > +<controller type='scsi' index='0' model='virtio-scsi'/> > +<controller type='scsi' index='1' model='lsilogic'/> > +<controller type='scsi' index='2' model='lsilogic'/> > +<controller type='scsi' index='3' model='virtio-scsi'/> > +<memballoon model='virtio'/> > +</devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index fdbe95a..cc1ac44 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -463,6 +463,9 @@ mymain(void) > QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); > DO_TEST("disk-scsi-device-auto", false, > QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); > + DO_TEST("disk-scsi-disk-split", false, > + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, > + QEMU_CAPS_SCSI_CD); > DO_TEST("disk-scsi-vscsi", false, > QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); > DO_TEST("disk-scsi-virtio-scsi", false, ACK BTW, related to this, one of these days we should fix our handling of <disk type="block">, so that it explicitly sets host_device, host_cdrom or host_floppy as appropriate.