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
