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@xxxxxxxxxx> 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list