Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
Avi Kivity a...@redhat.com writes: On 05/28/2010 10:24 PM, Luiz Capitulino wrote: If a password is needed, we should throw an error and let the QMP client set the password and try again. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Is the password some kind of global or per-monitor property? In that case it doesn't work with parallel execution of commands; better to have a password field (or assign IDs to passwords and require a passwordid=... argument). It sets the password in the host BlockDriverState. Which must already exist, i.e. you do it after blockdev_add. What happens if the guest device accesses the host drive before the key is set? Anything wrong with passing the password as argument? Did we avoid that to protect naive users from exposing their password via argv[]? That argument doesn't apply to QMP.
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
On Mon, 31 May 2010 13:05:37 +0200 Markus Armbruster arm...@redhat.com wrote: Avi Kivity a...@redhat.com writes: On 05/28/2010 10:24 PM, Luiz Capitulino wrote: If a password is needed, we should throw an error and let the QMP client set the password and try again. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Is the password some kind of global or per-monitor property? In that case it doesn't work with parallel execution of commands; better to have a password field (or assign IDs to passwords and require a passwordid=... argument). It sets the password in the host BlockDriverState. Which must already exist, i.e. you do it after blockdev_add. What happens if the guest device accesses the host drive before the key is set? It's supposed to fail, right Kevin? Anything wrong with passing the password as argument? Did we avoid that to protect naive users from exposing their password via argv[]? That argument doesn't apply to QMP.
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
Am 31.05.2010 15:48, schrieb Luiz Capitulino: On Mon, 31 May 2010 13:05:37 +0200 Markus Armbruster arm...@redhat.com wrote: Avi Kivity a...@redhat.com writes: On 05/28/2010 10:24 PM, Luiz Capitulino wrote: If a password is needed, we should throw an error and let the QMP client set the password and try again. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Is the password some kind of global or per-monitor property? In that case it doesn't work with parallel execution of commands; better to have a password field (or assign IDs to passwords and require a passwordid=... argument). It sets the password in the host BlockDriverState. Which must already exist, i.e. you do it after blockdev_add. What happens if the guest device accesses the host drive before the key is set? It's supposed to fail, right Kevin? I don't think it's a situation that is even supposed to happen. Which is exactly why I proposed an additional field to avoid it in the first place. As far as I know in the old monitor the user would be prompted for the password and as long as he doesn't enter it the VM is stopped, so we don't get in the same situation there. But if you enter a wrong password (I'm not sure if it's the same), it just seems to read garbage. Kevin
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
On 05/28/2010 10:24 PM, Luiz Capitulino wrote: If a password is needed, we should throw an error and let the QMP client set the password and try again. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Is the password some kind of global or per-monitor property? In that case it doesn't work with parallel execution of commands; better to have a password field (or assign IDs to passwords and require a passwordid=... argument). -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
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, transguest, 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 readonlyboth 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
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
On Fri, 28 May 2010 14:17:07 -0500 Anthony Liguori anth...@codemonkey.ws wrote: 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, transguest, 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 readonlyboth 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. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Regards, Anthony Liguori Would it make s1ense to add a password field to blockdev_add/change to avoid this? Kevin