Re: [libvirt] [PATCH] block: Set cdrom device read only flag

Kevin Wolf <kwolf(a)redhat.com&gt; 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.