From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37929 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OI53X-0003lD-9c for qemu-devel@nongnu.org; Fri, 28 May 2010 15:17:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OI53Q-0006Wl-9g for qemu-devel@nongnu.org; Fri, 28 May 2010 15:17:19 -0400 Received: from mail-iw0-f173.google.com ([209.85.214.173]:49627) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OI53Q-0006Wd-35 for qemu-devel@nongnu.org; Fri, 28 May 2010 15:17:12 -0400 Received: by iwn41 with SMTP id 41so101966iwn.4 for ; Fri, 28 May 2010 12:17:11 -0700 (PDT) Message-ID: <4C0016B3.6000505@codemonkey.ws> Date: Fri, 28 May 2010 14:17:07 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs References: <4C0015E6.50805@redhat.com> In-Reply-To: <4C0015E6.50805@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Luiz Capitulino , Gerd Hoffmann , Markus Armbruster , Avi Kivity On 05/28/2010 02:13 PM, Kevin Wolf wrote: > Am 28.05.2010 20:21, schrieb Markus Armbruster: > >> I'd like to give posting documentation of new QMP commands for review >> before posting code a try. But first let me explain briefly why we need >> new commands. >> >> We want a clean separation between host part (blockdev_add) and guest >> part (device_add). Existing -drive and drive_add don't provide that; >> they were designed to specify both parts together. Moreover, drive_add >> is limited to adding virtio drives (with pci_add's help) and SCSI >> drives. >> >> Support for defining just a host part for use with -device and was >> grafted onto -drive (if=none), but it's a mess. Some parts are >> redundant, other parts are broken. >> >> For instance, unit, bus, index, addr are redundant: -device does not use >> them, it provides its own parameters to specify bus and bus-specific >> address. >> >> The checks whether rerror, werror, readonly, cyls, heads, secs are sane >> for a particular guest driver are broken. The checks are in the -drive >> code, which used to know the guest driver, but doesn't with if=none. >> >> Additionally, removable media are flawed. Many parameters set with >> -drive silently revert to defaults on media change. >> >> My proposed solution is a new option -blockdev and monitor command >> blockdev_add. These specify only the host drive. Guest drive >> properties are left to -device / device_add. We keep -drive for >> backwards compatibility and command line convenience. Except we get rid >> of if=none (may need a grace period). >> >> New monitor command blockdev_del works regardless of how the host block >> device was created. >> >> New monitor command blockdev_change provides full control over the host >> part, unlike the existing change command. >> >> Summary of the host / guest split: >> >> -drive options host or guest? >> bus, unit, if, index, addr guest, already covered by qdev >> cyls, heads, secs, trans guest, new qdev properties >> (but defaults depend on image) >> media guest >> snapshot, file, cache, aio, format host, blockdev_add options >> rerror, werror host, guest drivers will reject >> values they don't support >> serial guest, new qdev properties >> readonly both host& guest, qdev will refuse >> to connect readonly host to read/ >> write guest >> >> QMP command docs: >> >> blockdev_add >> ------------ >> >> Add host block device. >> >> Arguments: >> >> - "id": the host block device's ID, must be unique (json-string) >> - "file": the disk image file to use (json-string, optional) >> - "format": disk format (json-string, optional) >> - Possible values: "raw", "qcow2", ... >> - "aio": host AIO (json-string, optional) >> - Possible values: "threads" (default), "native" >> - "cache": host cache usage (json-string, optional) >> - Possible values: "writethrough" (default), "writeback", "unsafe", >> "none" >> - "readonly": open image read-only (json-bool, optional, default false) >> - "rerror": what to do on read error (json-string, optional) >> - Possible values: "report" (default), "ignore", "stop" >> - "werror": what to do on write error (json-string, optional) >> - Possible values: "enospc" (default), "report", "ignore", "stop" >> - "snapshot": enable snapshot (json-bool, optional, default false) >> >> Example: >> >> -> { "execute": "blockdev_add", >> "arguments": { "format": "raw", "id": "blk1", >> "file": "fedora.img" } } >> <- { "return": {} } >> >> Notes: >> >> (1) If argument "file" is missing, all other optional arguments must be >> missing as well. This defines a block device with no media >> inserted. >> >> (2) It's possible to list supported disk formats by running QEMU with >> arguments "-blockdev_add \?". >> > -blockdev without _add you probably mean, if it's a command line option. > > Maybe one more thing to consider is encrypted images. With "change" in > the user monitor you're automatically prompted for the password. > > I'm not sure how this is supposed to work with QMP. From the > do_change_block code it looks as if you'd get an error and had to send a > block_set_passwd as a response to that. In the meantime the image would > be kind of half-open? What do devices do with it until the key is provided? > If a password is needed, we should throw an error and let the QMP client set the password and try again. Regards, Anthony Liguori > Would it make s1ense to add a password field to blockdev_add/change to > avoid this? > > Kevin > >