Re: [libvirt] [PATCH] block: Set cdrom device read only flag
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>
Cheers, Kevin.
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. --- 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");
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
On Thu, Aug 09, 2012 at 10:42:51AM +0200, Kevin Wolf wrote:
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
So qmp_change_blockdev uses bdrv_is_read_only() to check whether to try and open the backing file read only, which uses the ->read_only member of struct BlockDriverState to decide whether to pass the BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open(). I would assume we want to set this flag in drive_init() when the block driver state is initialised. How about a patch like this instead? diff --git a/blockdev.c b/blockdev.c index 8669142..ba22064 100644 --- a/blockdev.c +++ b/blockdev.c @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if_name[type], mediastr, unit_id); } dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv->read_only = ro; dinfo->devaddr = devaddr; dinfo->type = type; dinfo->bus = bus_id;
Am 12.08.2012 04:48, schrieb Kevin Shanahan:
So qmp_change_blockdev uses bdrv_is_read_only() to check whether to try and open the backing file read only, which uses the ->read_only member of struct BlockDriverState to decide whether to pass the BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open().
I would assume we want to set this flag in drive_init() when the block driver state is initialised. How about a patch like this instead?
diff --git a/blockdev.c b/blockdev.c index 8669142..ba22064 100644 --- a/blockdev.c +++ b/blockdev.c @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if_name[type], mediastr, unit_id); } dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv->read_only = ro; dinfo->devaddr = devaddr; dinfo->type = type; dinfo->bus = bus_id;
Ah, yes, this looks much more like the proper fix. Basically we need to set everything that is retained after a 'change' command. We have this code in qmp_change_blockdev(): bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp); bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot() additionally requires bs->open_flags to be right. Markus, how will this look in the -blockdev world? There seem to be properties that belong to host state, but are not coupled to a medium. Kevin
Kevin Wolf <kwolf@redhat.com> writes:
Am 12.08.2012 04:48, schrieb Kevin Shanahan:
So qmp_change_blockdev uses bdrv_is_read_only() to check whether to try and open the backing file read only, which uses the ->read_only member of struct BlockDriverState to decide whether to pass the BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open().
I would assume we want to set this flag in drive_init() when the block driver state is initialised. How about a patch like this instead?
diff --git a/blockdev.c b/blockdev.c index 8669142..ba22064 100644 --- a/blockdev.c +++ b/blockdev.c @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if_name[type], mediastr, unit_id); } dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv->read_only = ro; dinfo->devaddr = devaddr; dinfo->type = type; dinfo->bus = bus_id;
Ah, yes, this looks much more like the proper fix. Basically we need to set everything that is retained after a 'change' command. We have this code in qmp_change_blockdev():
bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot() additionally requires bs->open_flags to be right.
Markus, how will this look in the -blockdev world? There seem to be properties that belong to host state, but are not coupled to a medium.
Really? Read-only is clearly a property of the medium. There's a separate read-only belonging to the device. A CD-ROM medium is read-only, even when loaded in an optical disk drive that can burn. An optical disk drive that can't burn is read-only, even when writable medium is loaded. Here's how I believe it should work: * BDS member read_only describes the medium. * block.h gets it right: you specify read-only with bdrv_open(), not with bdrv_new(). * -drive gets it right: parameter readonly gets ignored unless we're defining media. Not nice: it's ignored silently. * Monitor command change gets it wrong: it doesn't let you specify the new medium's read-only-ness. Easy enough to fix for QMP, just add a suitable argument. Even better: create a new, non-multiplexed command, and let "change" rot in peace. Not sure about the best way to fix it in the human monitor. * Device model has its own read-only predicate. * If a device model comes in both a read-only and a read-write flavor, it should have a bool property "readonly". * A device model can only use a backend with a suitable media state (has media, is read-only, ...). For instance, ide-hd can't use a read-only backend. * Media change disabled while backend is attached to a device model with fixed media. * Convenient defaults (optional) * A device model property "readonly" could default to the media's read-only-ness. * Monitor command change could default to readonly when the backend is attached to a device model that can't write. * Both -drive and change could default to readonly when the image isn't writable. This leads to: 1. Replace monitor command change. 2. Optional: look for defaults to improve.
Am 13.08.2012 13:57, schrieb Markus Armbruster:
Kevin Wolf <kwolf@redhat.com> writes:
Am 12.08.2012 04:48, schrieb Kevin Shanahan:
So qmp_change_blockdev uses bdrv_is_read_only() to check whether to try and open the backing file read only, which uses the ->read_only member of struct BlockDriverState to decide whether to pass the BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open().
I would assume we want to set this flag in drive_init() when the block driver state is initialised. How about a patch like this instead?
diff --git a/blockdev.c b/blockdev.c index 8669142..ba22064 100644 --- a/blockdev.c +++ b/blockdev.c @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if_name[type], mediastr, unit_id); } dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv->read_only = ro; dinfo->devaddr = devaddr; dinfo->type = type; dinfo->bus = bus_id;
Ah, yes, this looks much more like the proper fix. Basically we need to set everything that is retained after a 'change' command. We have this code in qmp_change_blockdev():
bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot() additionally requires bs->open_flags to be right.
Markus, how will this look in the -blockdev world? There seem to be properties that belong to host state, but are not coupled to a medium.
Really?
Read-only is clearly a property of the medium. There's a separate read-only belonging to the device.
A CD-ROM medium is read-only, even when loaded in an optical disk drive that can burn.
An optical disk drive that can't burn is read-only, even when writable medium is loaded.
Here's how I believe it should work:
* BDS member read_only describes the medium.
* block.h gets it right: you specify read-only with bdrv_open(), not with bdrv_new().
* -drive gets it right: parameter readonly gets ignored unless we're defining media. Not nice: it's ignored silently.
* Monitor command change gets it wrong: it doesn't let you specify the new medium's read-only-ness.
Easy enough to fix for QMP, just add a suitable argument. Even better: create a new, non-multiplexed command, and let "change" rot in peace.
Not sure about the best way to fix it in the human monitor.
I think it's mostly the human monitor that needs this state. Of course, users will always want to have it, but in the case of QMP this work is offloaded to libvirt. So people expect that if they have started with a block device read-only, it remains read-only after they switch to a different image file ("change the medium"). They also expect that if they initially started with snapshot=on, the newly loaded image file will inherit this flag and be snapshotted as well. We know these expectations because 'change' didn't always retain these properties (or probably did so initially, but was broken at some point), and people complained, so it was fixed. So we can't just break it. This works today. Kevin's bug is about retaining the properties also when initially there's no medium in the drive, and it would probably be reasonable enough to say that we shouldn't lose them either when at runtime a medium is first ejected so that the drive is empty, and then a new medium is inserted. As always ignoring requirements is the easiest way, but it doesn't make users happy.
* Device model has its own read-only predicate.
* If a device model comes in both a read-only and a read-write flavor, it should have a bool property "readonly".
* A device model can only use a backend with a suitable media state (has media, is read-only, ...). For instance, ide-hd can't use a read-only backend.
* Media change disabled while backend is attached to a device model with fixed media.
* Convenient defaults (optional)
* A device model property "readonly" could default to the media's read-only-ness.
* Monitor command change could default to readonly when the backend is attached to a device model that can't write.
* Both -drive and change could default to readonly when the image isn't writable.
This doesn't give us today's behaviour with devices that can do both read-only and r/w media. Most common case for it are probably CD-ROMs which can never write, so it comes relatively close at least (at least until someone implements burning CDs in our emulation...) It doesn't do anything about snapshot=on. Kevin
Am 13.08.2012 09:54, schrieb Kevin Wolf:
Am 12.08.2012 04:48, schrieb Kevin Shanahan:
So qmp_change_blockdev uses bdrv_is_read_only() to check whether to try and open the backing file read only, which uses the ->read_only member of struct BlockDriverState to decide whether to pass the BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open().
I would assume we want to set this flag in drive_init() when the block driver state is initialised. How about a patch like this instead?
diff --git a/blockdev.c b/blockdev.c index 8669142..ba22064 100644 --- a/blockdev.c +++ b/blockdev.c @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if_name[type], mediastr, unit_id); } dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv->read_only = ro; dinfo->devaddr = devaddr; dinfo->type = type; dinfo->bus = bus_id;
Ah, yes, this looks much more like the proper fix. Basically we need to set everything that is retained after a 'change' command. We have this code in qmp_change_blockdev():
bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot() additionally requires bs->open_flags to be right.
Kevin, would you like to resend this as a proper patch with a Signed-off-by line? If you like, you can also add a dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0. This would fix the second bug I mentioned above, that snapshot=on is lost on empty drives. Kevin
participants (4)
-
Kevin Shanahan -
Kevin Wolf -
Markus Armbruster -
ronnie sahlberg