Patchwork block: Set cdrom device read only flag

login
register
mail settings
Submitter Kevin Shanahan
Date Aug. 2, 2012, 7:20 a.m.
Message ID <20120802072040.GD18298@tuon.disenchant.local>
Download mbox | patch
Permalink /patch/174695/
State New
Headers show

Comments

Kevin Shanahan - Aug. 2, 2012, 7:20 a.m.
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.

Cheers,
Kevin.
Kevin Wolf - Aug. 6, 2012, 10:07 a.m.
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
Markus Armbruster - Aug. 7, 2012, 8:47 a.m.
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.
ronniesahlberg@gmail.com - Aug. 7, 2012, 9:46 a.m.
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.
>
Markus Armbruster - Aug. 7, 2012, 11:54 a.m.
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?
Kevin Wolf - Aug. 9, 2012, 8:42 a.m.
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

Patch

--- 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");