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>

Am 07.08.2012 10:47, schrieb Markus Armbruster: > Kevin Wolf <kwolf(a)redhat.com&gt; 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

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.

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.