| Message ID | 20120802072040.GD18298@tuon.disenchant.local |
|---|---|
| State | New |
| Headers | show |
Am 02.08.2012 09:20, schrieb Kevin Shanahan: > On Thu, Aug 02, 2012 at 02:49:52PM +0930, Kevin Shanahan wrote: >> On Thu, Aug 02, 2012 at 11:46:13AM +0930, Kevin Shanahan wrote: >>> On Thu, Aug 02, 2012 at 11:02:42AM +0930, Kevin Shanahan wrote: >>>> Set the block driver read_only flag for cdrom devices so that >>>> qmp_change_blockdev does not attempt to open cdrom files in read-write >>>> mode when changing media. >>> >>> Hrm, this fixes my simple test case using the kvm monitor directly but >>> changing media via libvirt still has the same issue (fails for RO >>> files, succeeds for writable files). >>> >>> $ virsh attach-disk --type cdrom --mode readonly test1 /srv/kvm/iso/ubuntu-12.04-server-amd64.iso hdc >>> error: Failed to attach disk >>> error: internal error unable to execute QEMU command 'change': Could not open '/srv/kvm/iso/ubuntu-12.04-server-amd64.iso' >>> >>> I'll keep looking into it. >> >> In the libvirt case, it seems libvirt is failing to add media=cdrom to >> the commandline, so in this case qemu is defaulting to media=disk and >> my proposed fix has no effect. Diving into libvirt now to see why no >> media=disk is getting added... >> >> Common test case has this xml (generated by virt-install): >> >> <disk type='block' device='cdrom'> >> <driver name='qemu' type='raw'/> >> <target dev='hdc' bus='ide'/> >> <readonly/> >> <alias name='ide0-1-0'/> >> <address type='drive' controller='0' bus='1' target='0' unit='0'/> >> </disk> > > Ok, looks like libvirt is intentionally leaving media=cdrom off the > command line in the case that "-device ide-cd,..." is > supported. Presumably by specifying the device this way, qemu is > supposed to work out that the media type is cdrom automatically (but > it doesn't, it defaults to disk). > > Libvirt wants to use: > > qemu-kvm ... \ > -drive if=none,id=drive-ide0-1-0,readonly=on,format=raw \ > -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 ... > > If I hack qemu/qemu_command.c::qemuBuildDriveStr() to ignore the check > for QEMU_CAPS_IDE_CD and always add media=cdrom, then (with my qemu as > well patch) qemu will open cdrom media files read-only. > > There's probably a neater way to just get qemu to set the media type > if "-device ide-cd,..." is used, but I haven't worked it out yet. > > Anyway, apologies for the rambling conversation with myself on your > lists. Hope this is helpful in some way. Thanks, that's some good information. However, I don't think you should start from media=cdrom. libvirt already does specify readonly=on and that is the property you're really interested in. Since commit 528f7663 (released with 0.13) the 'change' command should keep the read-only flag for all kinds of media. Now I'm not sure where you lost the read-only flag. At least on git master it doesn't seem to reproduce for me. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.08.2012 09:20, schrieb Kevin Shanahan: >> On Thu, Aug 02, 2012 at 02:49:52PM +0930, Kevin Shanahan wrote: >>> On Thu, Aug 02, 2012 at 11:46:13AM +0930, Kevin Shanahan wrote: >>>> On Thu, Aug 02, 2012 at 11:02:42AM +0930, Kevin Shanahan wrote: >>>>> Set the block driver read_only flag for cdrom devices so that >>>>> qmp_change_blockdev does not attempt to open cdrom files in read-write >>>>> mode when changing media. >>>> >>>> Hrm, this fixes my simple test case using the kvm monitor directly but >>>> changing media via libvirt still has the same issue (fails for RO >>>> files, succeeds for writable files). >>>> >>>> $ virsh attach-disk --type cdrom --mode readonly test1 >>>> /srv/kvm/iso/ubuntu-12.04-server-amd64.iso hdc >>>> error: Failed to attach disk >>>> error: internal error unable to execute QEMU command 'change': >>>> Could not open '/srv/kvm/iso/ubuntu-12.04-server-amd64.iso' >>>> >>>> I'll keep looking into it. >>> >>> In the libvirt case, it seems libvirt is failing to add media=cdrom to >>> the commandline, so in this case qemu is defaulting to media=disk and >>> my proposed fix has no effect. Diving into libvirt now to see why no >>> media=disk is getting added... >>> >>> Common test case has this xml (generated by virt-install): >>> >>> <disk type='block' device='cdrom'> >>> <driver name='qemu' type='raw'/> >>> <target dev='hdc' bus='ide'/> >>> <readonly/> >>> <alias name='ide0-1-0'/> >>> <address type='drive' controller='0' bus='1' target='0' unit='0'/> >>> </disk> >> >> Ok, looks like libvirt is intentionally leaving media=cdrom off the >> command line in the case that "-device ide-cd,..." is >> supported. Presumably by specifying the device this way, qemu is >> supposed to work out that the media type is cdrom automatically (but >> it doesn't, it defaults to disk). >> >> Libvirt wants to use: >> >> qemu-kvm ... \ >> -drive if=none,id=drive-ide0-1-0,readonly=on,format=raw \ >> -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 ... >> >> If I hack qemu/qemu_command.c::qemuBuildDriveStr() to ignore the check >> for QEMU_CAPS_IDE_CD and always add media=cdrom, then (with my qemu as >> well patch) qemu will open cdrom media files read-only. >> >> There's probably a neater way to just get qemu to set the media type >> if "-device ide-cd,..." is used, but I haven't worked it out yet. >> >> Anyway, apologies for the rambling conversation with myself on your >> lists. Hope this is helpful in some way. > > Thanks, that's some good information. > > However, I don't think you should start from media=cdrom. libvirt > already does specify readonly=on and that is the property you're really > interested in. Since commit 528f7663 (released with 0.13) the 'change' > command should keep the read-only flag for all kinds of media. Correct. > Now I'm not sure where you lost the read-only flag. At least on git > master it doesn't seem to reproduce for me. I can: $ qemu --enable-kvm -S -m 384 -vnc localhost:5500 -monitor stdio \ -drive if=none,id=cd,readonly=on,format=raw \ -device ide-cd,bus=ide.1,unit=0,drive=cd QEMU 1.1.50 monitor - type 'help' for more information (qemu) change cd r5.iso Could not open 'r5.iso' (qemu) q armbru@blackfin:~/work/images$ ls -l r5.iso -r--r--r--. 1 armbru armbru 2872639488 Mar 31 2011 r5.iso Looks like a QEMU bug to me.
Since pretty much every cdrom drive use scsi today, shouldnt the readonly/writeable flag for MMC devices rather be done down in the hw/scsi* code rather than the generic block code? If/when/ever I get enough time I would like to port the writeable dvd+r emulation I wrote for STGT to qemu. In STGT, MMC/DVD devices can be either read-only or read-write. If the filesize for the backing file is >0 bytes, it is assumed the file is an iso image and that the file is a read-only iso image. If filesize is ==0, then the file is opened read-write and is treated as a "blank dvd+r disk that the initiator can write/burn to" regards ronnie sahlberg On Tue, Aug 7, 2012 at 6:47 PM, Markus Armbruster <armbru@redhat.com> wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 02.08.2012 09:20, schrieb Kevin Shanahan: >>> On Thu, Aug 02, 2012 at 02:49:52PM +0930, Kevin Shanahan wrote: >>>> On Thu, Aug 02, 2012 at 11:46:13AM +0930, Kevin Shanahan wrote: >>>>> On Thu, Aug 02, 2012 at 11:02:42AM +0930, Kevin Shanahan wrote: >>>>>> Set the block driver read_only flag for cdrom devices so that >>>>>> qmp_change_blockdev does not attempt to open cdrom files in read-write >>>>>> mode when changing media. >>>>> >>>>> Hrm, this fixes my simple test case using the kvm monitor directly but >>>>> changing media via libvirt still has the same issue (fails for RO >>>>> files, succeeds for writable files). >>>>> >>>>> $ virsh attach-disk --type cdrom --mode readonly test1 >>>>> /srv/kvm/iso/ubuntu-12.04-server-amd64.iso hdc >>>>> error: Failed to attach disk >>>>> error: internal error unable to execute QEMU command 'change': >>>>> Could not open '/srv/kvm/iso/ubuntu-12.04-server-amd64.iso' >>>>> >>>>> I'll keep looking into it. >>>> >>>> In the libvirt case, it seems libvirt is failing to add media=cdrom to >>>> the commandline, so in this case qemu is defaulting to media=disk and >>>> my proposed fix has no effect. Diving into libvirt now to see why no >>>> media=disk is getting added... >>>> >>>> Common test case has this xml (generated by virt-install): >>>> >>>> <disk type='block' device='cdrom'> >>>> <driver name='qemu' type='raw'/> >>>> <target dev='hdc' bus='ide'/> >>>> <readonly/> >>>> <alias name='ide0-1-0'/> >>>> <address type='drive' controller='0' bus='1' target='0' unit='0'/> >>>> </disk> >>> >>> Ok, looks like libvirt is intentionally leaving media=cdrom off the >>> command line in the case that "-device ide-cd,..." is >>> supported. Presumably by specifying the device this way, qemu is >>> supposed to work out that the media type is cdrom automatically (but >>> it doesn't, it defaults to disk). >>> >>> Libvirt wants to use: >>> >>> qemu-kvm ... \ >>> -drive if=none,id=drive-ide0-1-0,readonly=on,format=raw \ >>> -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 ... >>> >>> If I hack qemu/qemu_command.c::qemuBuildDriveStr() to ignore the check >>> for QEMU_CAPS_IDE_CD and always add media=cdrom, then (with my qemu as >>> well patch) qemu will open cdrom media files read-only. >>> >>> There's probably a neater way to just get qemu to set the media type >>> if "-device ide-cd,..." is used, but I haven't worked it out yet. >>> >>> Anyway, apologies for the rambling conversation with myself on your >>> lists. Hope this is helpful in some way. >> >> Thanks, that's some good information. >> >> However, I don't think you should start from media=cdrom. libvirt >> already does specify readonly=on and that is the property you're really >> interested in. Since commit 528f7663 (released with 0.13) the 'change' >> command should keep the read-only flag for all kinds of media. > > Correct. > >> Now I'm not sure where you lost the read-only flag. At least on git >> master it doesn't seem to reproduce for me. > > I can: > > $ qemu --enable-kvm -S -m 384 -vnc localhost:5500 -monitor stdio \ > -drive if=none,id=cd,readonly=on,format=raw \ > -device ide-cd,bus=ide.1,unit=0,drive=cd > QEMU 1.1.50 monitor - type 'help' for more information > (qemu) change cd r5.iso > Could not open 'r5.iso' > (qemu) q > armbru@blackfin:~/work/images$ ls -l r5.iso > -r--r--r--. 1 armbru armbru 2872639488 Mar 31 2011 r5.iso > > Looks like a QEMU bug to me. >
ronnie sahlberg <ronniesahlberg@gmail.com> writes: > Since pretty much every cdrom drive use scsi today, shouldnt the > readonly/writeable flag for MMC devices rather > be done down in the hw/scsi* code rather than the generic block code? There are two separate things that can be read-only: device models and BlockDriverStates. -drive's parameter readonly applies to the top BlockDriverState. Some device models default their read-only-ness to their BlockDriverState's. Examples: * Device models ide-cd and scsi-cd are always read-only. They don't care whether the BlockDriverState that backs them is. * Device model ide-hd is always read-write. It fails initialization when its BlockDriverState is read-only. * Device model scsi-hd supports both read-only and read-write. It's read-only iff its BlockDriverState is. * -drive if={ide,scsi},media=cdrom implies readonly=on, and creates an {ide,scsi}-cd device. * -drive if={ide,scsi},media=disk creates an {ide,scsi}-hd device (media=disk is the default). * -drive without readonly=on fails when the image isn't writable. > If/when/ever I get enough time I would like to port the writeable > dvd+r emulation I wrote for STGT to qemu. > > > In STGT, MMC/DVD devices can be either read-only or read-write. > If the filesize for the backing file is >0 bytes, it is assumed the > file is an iso image and that the file is a read-only iso image. > If filesize is ==0, then the file is opened read-write and is treated > as a "blank dvd+r disk that the initiator can write/burn to" I doubt keying on the backing file size is a good idea, too obscure. Why would keying on the BlockDriverState's read-only-ness not work?
Am 07.08.2012 10:47, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 02.08.2012 09:20, schrieb Kevin Shanahan: >>> On Thu, Aug 02, 2012 at 02:49:52PM +0930, Kevin Shanahan wrote: >>>> On Thu, Aug 02, 2012 at 11:46:13AM +0930, Kevin Shanahan wrote: >>>>> On Thu, Aug 02, 2012 at 11:02:42AM +0930, Kevin Shanahan wrote: >>>>>> Set the block driver read_only flag for cdrom devices so that >>>>>> qmp_change_blockdev does not attempt to open cdrom files in read-write >>>>>> mode when changing media. >>>>> >>>>> Hrm, this fixes my simple test case using the kvm monitor directly but >>>>> changing media via libvirt still has the same issue (fails for RO >>>>> files, succeeds for writable files). >>>>> >>>>> $ virsh attach-disk --type cdrom --mode readonly test1 >>>>> /srv/kvm/iso/ubuntu-12.04-server-amd64.iso hdc >>>>> error: Failed to attach disk >>>>> error: internal error unable to execute QEMU command 'change': >>>>> Could not open '/srv/kvm/iso/ubuntu-12.04-server-amd64.iso' >>>>> >>>>> I'll keep looking into it. >>>> >>>> In the libvirt case, it seems libvirt is failing to add media=cdrom to >>>> the commandline, so in this case qemu is defaulting to media=disk and >>>> my proposed fix has no effect. Diving into libvirt now to see why no >>>> media=disk is getting added... >>>> >>>> Common test case has this xml (generated by virt-install): >>>> >>>> <disk type='block' device='cdrom'> >>>> <driver name='qemu' type='raw'/> >>>> <target dev='hdc' bus='ide'/> >>>> <readonly/> >>>> <alias name='ide0-1-0'/> >>>> <address type='drive' controller='0' bus='1' target='0' unit='0'/> >>>> </disk> >>> >>> Ok, looks like libvirt is intentionally leaving media=cdrom off the >>> command line in the case that "-device ide-cd,..." is >>> supported. Presumably by specifying the device this way, qemu is >>> supposed to work out that the media type is cdrom automatically (but >>> it doesn't, it defaults to disk). >>> >>> Libvirt wants to use: >>> >>> qemu-kvm ... \ >>> -drive if=none,id=drive-ide0-1-0,readonly=on,format=raw \ >>> -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 ... >>> >>> If I hack qemu/qemu_command.c::qemuBuildDriveStr() to ignore the check >>> for QEMU_CAPS_IDE_CD and always add media=cdrom, then (with my qemu as >>> well patch) qemu will open cdrom media files read-only. >>> >>> There's probably a neater way to just get qemu to set the media type >>> if "-device ide-cd,..." is used, but I haven't worked it out yet. >>> >>> Anyway, apologies for the rambling conversation with myself on your >>> lists. Hope this is helpful in some way. >> >> Thanks, that's some good information. >> >> However, I don't think you should start from media=cdrom. libvirt >> already does specify readonly=on and that is the property you're really >> interested in. Since commit 528f7663 (released with 0.13) the 'change' >> command should keep the read-only flag for all kinds of media. > > Correct. > >> Now I'm not sure where you lost the read-only flag. At least on git >> master it doesn't seem to reproduce for me. > > I can: > > $ qemu --enable-kvm -S -m 384 -vnc localhost:5500 -monitor stdio \ > -drive if=none,id=cd,readonly=on,format=raw \ > -device ide-cd,bus=ide.1,unit=0,drive=cd > QEMU 1.1.50 monitor - type 'help' for more information > (qemu) change cd r5.iso > Could not open 'r5.iso' > (qemu) q > armbru@blackfin:~/work/images$ ls -l r5.iso > -r--r--r--. 1 armbru armbru 2872639488 Mar 31 2011 r5.iso > > Looks like a QEMU bug to me. Right, now it breaks for me as well. The difference is that when I tried on Monday, I didn't use an empty drive. Kevin
--- libvirt-0.9.13.orig/src/qemu/qemu_command.c.orig 2012-08-02 16:45:25.000000000 +0930 +++ libvirt-0.9.13.orig/src/qemu/qemu_command.c 2012-08-02 16:46:11.000000000 +0930 @@ -2082,7 +2082,7 @@ if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) virBufferAddLit(&opt, ",media=cdrom"); } else if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_IDE_CD)) + //if (!qemuCapsGet(qemuCaps, QEMU_CAPS_IDE_CD)) virBufferAddLit(&opt, ",media=cdrom"); } else { virBufferAddLit(&opt, ",media=cdrom");