[Qemu-devel] RFC: blockdev_add & friends, brief rationale, QMP docs

[Qemu-devel] RFC: blockdev_add & friends, brief rationale, QMP docs

[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_del
------------

Remove a host block device.

Arguments:

- "id": the host block device's ID (json-string)

Example:

-> { "execute": "blockdev_del", "arguments": { "id": "blk1" } }
<- { "return": {} }


blockdev_change
---------------

Change host block device media.

Arguments are exactly like blockdev_add.

Notes:

(1) If argument "file" is missing, all other optional arguments must be
    missing as well.  This ejects the media without inserting a new one.

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Kevin Wolf]

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?

Would it make sense 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

[Anthony Liguori]

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
>
>    

Re: [Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Luiz Capitulino]

On Fri, 28 May 2010 14:17:07 -0500
Anthony Liguori <anthony@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, 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.

 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
> >
> >    
> 

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Avi Kivity]

On 05/28/2010 09:21 PM, Markus Armbruster wrote:

<snip, agreed>

> 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
>    

We expose some of the cache property to the guest.  IMO we need the 
cache property to be both guest and host, with qemu bridging the 
impedance mismatch if needed.

> rerror, werror                      host, guest drivers will reject
>                                        values they don't support
>    

Did you mean 'block format drivers will reject'?

> 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)
>    

Unique in which namespace?  A global ID namespace if fine.

> - "file": the disk image file to use (json-string, optional)
> - "format": disk format (json-string, optional)
>      - Possible values: "raw", "qcow2", ...
>    

Need some way to list supported formats.

> - "aio": host AIO (json-string, optional)
>      - Possible values: "threads" (default), "native"
>    

Need some way to list supported options.

> - "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)
>
>    

An alternative to the "Need some way to list *" is to provide another 
query capability, akin to KVM_CAP_..., but I think listing is superior.

> 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 \?".
>    

Ok, so there's a partial answer here.

> blockdev_change
> ---------------
>
> Change host block device media.
>
> Arguments are exactly like blockdev_add.
>
> Notes:
>
> (1) If argument "file" is missing, all other optional arguments must be
>      missing as well.  This ejects the media without inserting a new one.
>    

Maybe we want an explicit blockdev_eject instead, not sure.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Avi Kivity]

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

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Avi Kivity <avi@redhat.com> writes:

> On 05/28/2010 09:21 PM, Markus Armbruster wrote:
>
> <snip, agreed>
>
>> 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
>>    
>
> We expose some of the cache property to the guest.  IMO we need the
> cache property to be both guest and host, with qemu bridging the
> impedance mismatch if needed.

Yes.

How should the device properties look like?

>> rerror, werror                      host, guest drivers will reject
>>                                        values they don't support
>>    
>
> Did you mean 'block format drivers will reject'?

No.  Actual implementation is in the guest drivers,
e.g. ide_handle_rw_error().

I see this as the host outsourcing the actual work to guest drivers.
Guest drivers that can't do the work should complain.  Right now, they
silently ignore the order.

>> 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)
>>    
>
> Unique in which namespace?  A global ID namespace if fine.

The device ID namespace.

We have numerous ID namespaces already: each QemuOptsList, device IDs
(which happens to be a superset of QemuOptsList qemu_device_opts),
perhaps more.

>> - "file": the disk image file to use (json-string, optional)
>> - "format": disk format (json-string, optional)
>>      - Possible values: "raw", "qcow2", ...
>>    
>
> Need some way to list supported formats.

See below.

>> - "aio": host AIO (json-string, optional)
>>      - Possible values: "threads" (default), "native"
>>    
>
> Need some way to list supported options.
>
>> - "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)
>>
>>    
>
> An alternative to the "Need some way to list *" is to provide another
> query capability, akin to KVM_CAP_..., but I think listing is
> superior.
>
>> 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 \?".
>>    
>
> Ok, so there's a partial answer here.

For human users, we need accurate interactive help, both for fixed sets
of options like "cache", and for configurable sets like "format".
-blockdev_add \? simply follows existing usage there.

We need to make QMP "self-documenting": describe commands, arguments,
argument values and so forth with sufficient completeness.  In
particular, for arguments that are enumerations, enumerate all possible
values.

>> blockdev_change
>> ---------------
>>
>> Change host block device media.
>>
>> Arguments are exactly like blockdev_add.
>>
>> Notes:
>>
>> (1) If argument "file" is missing, all other optional arguments must be
>>      missing as well.  This ejects the media without inserting a new one.
>>    
>
> Maybe we want an explicit blockdev_eject instead, not sure.

Either blockdev_change (can eject, insert with auto-eject) or completely
orthogonal blockdev_eject (can only eject) + blockdev_insert (can only
insert, no auto-eject), I'd say.

Thanks for the review!

Re: [Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Avi Kivity <avi@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.

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Avi Kivity]

On 05/31/2010 01:54 PM, Markus Armbruster wrote:
>
>> We expose some of the cache property to the guest.  IMO we need the
>> cache property to be both guest and host, with qemu bridging the
>> impedance mismatch if needed.
>>      
> Yes.
>
> How should the device properties look like?
>    

write_cache=enabled|disabled|none? (disabled can be enabled by the 
guest, but none cannot)
barrier=supported|unsupported?

Need to look at our supported interfaces and see what's the LCM.

>>> rerror, werror                      host, guest drivers will reject
>>>                                         values they don't support
>>>
>>>        
>> Did you mean 'block format drivers will reject'?
>>      
> No.  Actual implementation is in the guest drivers,
> e.g. ide_handle_rw_error().
>    

That is not a guest driver.  It is a device model.  A guest driver is 
something you modprobe.

> I see this as the host outsourcing the actual work to guest drivers.
> Guest drivers that can't do the work should complain.  Right now, they
> silently ignore the order.
>    

With the terminology change, it makes a bit of sense.

>
>> Maybe we want an explicit blockdev_eject instead, not sure.
>>      
> Either blockdev_change (can eject, insert with auto-eject) or completely
> orthogonal blockdev_eject (can only eject) + blockdev_insert (can only
> insert, no auto-eject), I'd say.
>    

I prefer the latter, especially as eject has numerous variants (software 
locked eject button, force=true to unwrap paper clip and insert into 
pinhole, tray ejects violently knocking down hot beverage, etc).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Avi Kivity <avi@redhat.com> writes:

> On 05/31/2010 01:54 PM, Markus Armbruster wrote:
>>
>>> We expose some of the cache property to the guest.  IMO we need the
>>> cache property to be both guest and host, with qemu bridging the
>>> impedance mismatch if needed.
>>>      
>> Yes.
>>
>> How should the device properties look like?
>>    
>
> write_cache=enabled|disabled|none? (disabled can be enabled by the
> guest, but none cannot)
> barrier=supported|unsupported?
>
> Need to look at our supported interfaces and see what's the LCM.

I figure we Can flesh it out later.

>>>> rerror, werror                      host, guest drivers will reject
>>>>                                         values they don't support
>>>>
>>>>        
>>> Did you mean 'block format drivers will reject'?
>>>      
>> No.  Actual implementation is in the guest drivers,
>> e.g. ide_handle_rw_error().
>>    
>
> That is not a guest driver.  It is a device model.  A guest driver is
> something you modprobe.

Sorry, sloppy QEMU terminology seeping into my writing :(

>> I see this as the host outsourcing the actual work to guest drivers.
>> Guest drivers that can't do the work should complain.  Right now, they
>> silently ignore the order.
>>    
>
> With the terminology change, it makes a bit of sense.
>
>>
>>> Maybe we want an explicit blockdev_eject instead, not sure.
>>>      
>> Either blockdev_change (can eject, insert with auto-eject) or completely
>> orthogonal blockdev_eject (can only eject) + blockdev_insert (can only
>> insert, no auto-eject), I'd say.
>>    
>
> I prefer the latter, especially as eject has numerous variants
> (software locked eject button, force=true to unwrap paper clip and
> insert into pinhole, tray ejects violently knocking down hot beverage,
> etc).

Okay.

Re: [Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Luiz Capitulino]

On Mon, 31 May 2010 13:05:37 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Avi Kivity <avi@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

[Kevin Wolf]

Am 31.05.2010 15:48, schrieb Luiz Capitulino:
> On Mon, 31 May 2010 13:05:37 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> Avi Kivity <avi@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

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Discussion with Christoph and Kevin uncovered yet another issue:
protocols.  I find it pretty confusing, but let me try to describe it
anyway; Christoph and Kevin, please correct my errors.

A host block device has a format.  A format has a name.

Below the format, it has a stack of protocols.  A protocol has a name
(with one exception), and may have protocol-specific arguments.

The most basic (and most commonly used) protocol is for accessing a
file.  Its argument is a file name.  It doesn't have a name.  Which
makes for ugly prose, so I'll call it "file".

Stacking protocols is somewhat exotic.  Think of stacking blkdebug on
top of another protocol, say nbd.

Our abstraction for formats is struct BlockDriver.

Our abstraction for protocols is also struct BlockDriver.  Except for
the special protocol "file", but that's detail.

Examples:

-drive file=foo.qcow2,format=qcow2

 Format "qcow2", protocol "file" with argument filename "foo.img"

-drive file=nbd:unix:/tmp/my_socket,format=raw

 Format "raw", protocol "nbd" with arguments domain "unix", filename
 "/tmp/my_socket"

-drive blkdebug:/tmp/blkdebug.cfg:fat:floppy:rw:/tmp/dir

 Format not specified (system guesses one), protocol "blkdebug" with
 argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
 arguments floppy true, dirname "/tmp/dir"

You see that -drive has a separate option for format, but has protocols
encoded in option file, in their own mini-language.  Doesn't work for
arbitrary filenames.  Besides, mini-languages to encode options in
strings are quite inappropriate for QMP.

So we need something cleaner for QMP.  Here's a sketch.  Instead of

- "file": the disk image file to use (json-string, optional)
- "format": disk format (json-string, optional)
    - Possible values: "raw", "qcow2", ...

have

- "format": disk format (json-string, optional)
    - Possible values: "raw", "qcow2", ...
- "protocol": json-array of json-object
  Each element object has a member "name"
    - Possible values: "file", "nbd", ...
  Additional members depend on the value of "name".
  For "name" = "file":
    - "file": file name (json-string)
  For "name" = "nbd":
    - "domain": address family (json-string, optional)
        - Possible values: "inet" (default), "unix"
    - "file": file name (json-string), only with "domain" = "unix"
    - "host": host name (json-string), only with "domain" = "inet"
    - "port": port (json-int), only with "domain" = "inet"
  ...

You get the idea.

Comments?

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Kevin Wolf]

Am 04.06.2010 16:16, schrieb Markus Armbruster:
> Discussion with Christoph and Kevin uncovered yet another issue:
> protocols.  I find it pretty confusing, but let me try to describe it
> anyway; Christoph and Kevin, please correct my errors.
> 
> A host block device has a format.  A format has a name.
> 
> Below the format, it has a stack of protocols.  A protocol has a name
> (with one exception), and may have protocol-specific arguments.
> 
> The most basic (and most commonly used) protocol is for accessing a
> file.  Its argument is a file name.  It doesn't have a name.  Which
> makes for ugly prose, so I'll call it "file".

It does have a name, and surprisingly it's called "file" indeed (defined
at block/raw-posix.c:744 for Linux).

> Stacking protocols is somewhat exotic.  Think of stacking blkdebug on
> top of another protocol, say nbd.

Considering that file is a protocol as well as nbd, it's any blkdebug
use that uses protocol stacking and therefore not that exotic - even
though not the most common case, of course.

> Our abstraction for formats is struct BlockDriver.
> 
> Our abstraction for protocols is also struct BlockDriver.  Except for
> the special protocol "file", but that's detail.

See above, file isn't really special.

> 
> Examples:
> 
> -drive file=foo.qcow2,format=qcow2
> 
>  Format "qcow2", protocol "file" with argument filename "foo.img"

Actually the protocol is guessed here. For this, not all protocols are
considered, it's only between file/host_device/host_cdrom/host_floppy
(these are the protocols implementing bdrv_probe_device, and file as the
default if no other protocol feels responsible)

> -drive file=nbd:unix:/tmp/my_socket,format=raw
> 
>  Format "raw", protocol "nbd" with arguments domain "unix", filename
>  "/tmp/my_socket"
> 
> -drive blkdebug:/tmp/blkdebug.cfg:fat:floppy:rw:/tmp/dir
> 
>  Format not specified (system guesses one), protocol "blkdebug" with
>  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
>  arguments floppy true, dirname "/tmp/dir"

These look right to me.

> 
> You see that -drive has a separate option for format, but has protocols
> encoded in option file, in their own mini-language.  Doesn't work for
> arbitrary filenames.  Besides, mini-languages to encode options in
> strings are quite inappropriate for QMP.
> 
> So we need something cleaner for QMP.  Here's a sketch.  Instead of
> 
> - "file": the disk image file to use (json-string, optional)
> - "format": disk format (json-string, optional)
>     - Possible values: "raw", "qcow2", ...
> 
> have
> 
> - "format": disk format (json-string, optional)
>     - Possible values: "raw", "qcow2", ...
> - "protocol": json-array of json-object
>   Each element object has a member "name"
>     - Possible values: "file", "nbd", ...
>   Additional members depend on the value of "name".
>   For "name" = "file":
>     - "file": file name (json-string)
>   For "name" = "nbd":
>     - "domain": address family (json-string, optional)
>         - Possible values: "inet" (default), "unix"
>     - "file": file name (json-string), only with "domain" = "unix"
>     - "host": host name (json-string), only with "domain" = "inet"
>     - "port": port (json-int), only with "domain" = "inet"
>   ...
> 
> You get the idea.
> 
> Comments?

Makes sense.

So blkdebug would define a field "protocol" (json-object) that it uses
to initialize the underlying protocol and we would get the stacking this
way?

Kevin

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.06.2010 16:16, schrieb Markus Armbruster:
>> Discussion with Christoph and Kevin uncovered yet another issue:
>> protocols.  I find it pretty confusing, but let me try to describe it
>> anyway; Christoph and Kevin, please correct my errors.
>> 
>> A host block device has a format.  A format has a name.
>> 
>> Below the format, it has a stack of protocols.  A protocol has a name
>> (with one exception), and may have protocol-specific arguments.
>> 
>> The most basic (and most commonly used) protocol is for accessing a
>> file.  Its argument is a file name.  It doesn't have a name.  Which
>> makes for ugly prose, so I'll call it "file".
>
> It does have a name, and surprisingly it's called "file" indeed (defined
> at block/raw-posix.c:744 for Linux).
>
>> Stacking protocols is somewhat exotic.  Think of stacking blkdebug on
>> top of another protocol, say nbd.
>
> Considering that file is a protocol as well as nbd, it's any blkdebug
> use that uses protocol stacking and therefore not that exotic - even
> though not the most common case, of course.
>
>> Our abstraction for formats is struct BlockDriver.
>> 
>> Our abstraction for protocols is also struct BlockDriver.  Except for
>> the special protocol "file", but that's detail.
>
> See above, file isn't really special.

I got confused by this code in bdrv_open_common():

    /* Open the image, either directly or using a protocol */
    if (drv->bdrv_file_open) {
        ret = drv->bdrv_file_open(bs, filename, open_flags);
    } else {
        ret = bdrv_file_open(&bs->file, filename, open_flags);
        if (ret >= 0) {
            ret = drv->bdrv_open(bs, open_flags);
        }
    }

When called by drive_init() via bdrv_open(), drv is the BlockDriver
named by format=F.  If drv->bdrv_file_open, is drv a format or a
protocol?  It's true for F in blkdebug, cow, http, https, ftp, ftps,
tftp, nbd, file, host_device, host_floppy, host_cdrom, vvfat, so it's a
protocol (except for cow, but Christoph tells me he's about to get that
one off the list).  But what's the format then?

The conditional's else seems clear: bdrv_file_open() finds the protocol
in filename (probes the file if missing), and then opens with that
protocol.

>> Examples:
>> 
>> -drive file=foo.qcow2,format=qcow2
>> 
>>  Format "qcow2", protocol "file" with argument filename "foo.img"
>
> Actually the protocol is guessed here. For this, not all protocols are
> considered, it's only between file/host_device/host_cdrom/host_floppy
> (these are the protocols implementing bdrv_probe_device, and file as the
> default if no other protocol feels responsible)

Then we need a way to ask for this guessing, say (pseudo-)protocol
"auto".

>> -drive file=nbd:unix:/tmp/my_socket,format=raw
>> 
>>  Format "raw", protocol "nbd" with arguments domain "unix", filename
>>  "/tmp/my_socket"
>> 
>> -drive blkdebug:/tmp/blkdebug.cfg:fat:floppy:rw:/tmp/dir
>> 
>>  Format not specified (system guesses one), protocol "blkdebug" with
>>  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
>>  arguments floppy true, dirname "/tmp/dir"
>
> These look right to me.
>
>> 
>> You see that -drive has a separate option for format, but has protocols
>> encoded in option file, in their own mini-language.  Doesn't work for
>> arbitrary filenames.  Besides, mini-languages to encode options in
>> strings are quite inappropriate for QMP.
>> 
>> So we need something cleaner for QMP.  Here's a sketch.  Instead of
>> 
>> - "file": the disk image file to use (json-string, optional)
>> - "format": disk format (json-string, optional)
>>     - Possible values: "raw", "qcow2", ...
>> 
>> have
>> 
>> - "format": disk format (json-string, optional)
>>     - Possible values: "raw", "qcow2", ...
>> - "protocol": json-array of json-object
>>   Each element object has a member "name"
>>     - Possible values: "file", "nbd", ...
>>   Additional members depend on the value of "name".
>>   For "name" = "file":
>>     - "file": file name (json-string)
>>   For "name" = "nbd":
>>     - "domain": address family (json-string, optional)
>>         - Possible values: "inet" (default), "unix"
>>     - "file": file name (json-string), only with "domain" = "unix"
>>     - "host": host name (json-string), only with "domain" = "inet"
>>     - "port": port (json-int), only with "domain" = "inet"
>>   ...
>> 
>> You get the idea.
>> 
>> Comments?
>
> Makes sense.
>
> So blkdebug would define a field "protocol" (json-object) that it uses
> to initialize the underlying protocol and we would get the stacking this
> way?

No, my proposal represents the stack of protocols as json-array, not by
nesting them.  I should have given examples.  Reusing my three examples
from above:

* Format "qcow2", protocol "auto" with argument filename "foo.img"

    "format": "qcow2",
    "protocol": [{ "name": "auto", "file": "foo.qcow2" }],

* Format "raw", protocol "nbd" with arguments domain "unix", filename
  "/tmp/my_socket"

    "format": "raw"
    "protocol": [{ "name": "nbd", "domain": "unix", "file": "/tmp/my_socket" }]

* Format not specified (system guesses one), protocol "blkdebug" with
  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
  arguments floppy true, dirname "/tmp/dir"

    "protocol": [{ "name": "blkdebug", "file": "/tmp/blkdebug.cfg" },
                 { "name": "fat", "floppy": true, "dir": "/tmp/dir" }]

With nesting, we'd get something like this instead:

* Format "qcow2", protocol "auto" with argument filename "foo.img"

    "format": "qcow2",
    "protocol": { "name": "auto", "file": "foo.qcow2" }

* Format "raw", protocol "nbd" with arguments domain "unix", filename
  "/tmp/my_socket"

    "format": "raw"
    "protocol": { "name": "nbd", "domain": "unix", "file": "/tmp/my_socket" }

* Format not specified (system guesses one), protocol "blkdebug" with
  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
  arguments floppy true, dirname "/tmp/dir"

    "protocol": { "name": "blkdebug", "file": "/tmp/blkdebug.cfg",
        "protocol": { "name": "fat", "floppy": true, "dir": "/tmp/dir" }
    }

Nesting has one small advantage, namely a protocol's property "stacks on
top of another protocol" becomes explicit in syntax: it's true iff it
has a "protocol" member.

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Kevin Wolf]

Am 04.06.2010 17:53, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 04.06.2010 16:16, schrieb Markus Armbruster:
>>> Our abstraction for formats is struct BlockDriver.
>>>
>>> Our abstraction for protocols is also struct BlockDriver.  Except for
>>> the special protocol "file", but that's detail.
>>
>> See above, file isn't really special.
> 
> I got confused by this code in bdrv_open_common():
> 
>     /* Open the image, either directly or using a protocol */
>     if (drv->bdrv_file_open) {
>         ret = drv->bdrv_file_open(bs, filename, open_flags);
>     } else {
>         ret = bdrv_file_open(&bs->file, filename, open_flags);
>         if (ret >= 0) {
>             ret = drv->bdrv_open(bs, open_flags);
>         }
>     }
> 
> When called by drive_init() via bdrv_open(), drv is the BlockDriver
> named by format=F.  If drv->bdrv_file_open, is drv a format or a
> protocol?  

A protocol. Formats use bdrv_open and get a BlockDriverState of the
underlying protocol as a parameter, whereas protocols use bdrv_file_open
and get the filename as a parameter.

> It's true for F in blkdebug, cow, http, https, ftp, ftps,
> tftp, nbd, file, host_device, host_floppy, host_cdrom, vvfat, so it's a
> protocol (except for cow, but Christoph tells me he's about to get that
> one off the list).  But what's the format then?

You're not supposed to use a protocol with format=F, even though
currently it works. Once cow is converted (it currently accesses the
file directly with POSIX functions instead of using the protocol) we can
make this an error so that it's not even possible to directly use a
protocol. Then the only way to get into the then branch is explicitly
calling bdrv_file_open.

To get the same behaviour as today with directly using a protocol,
you'll need to use raw as the format.

> The conditional's else seems clear: bdrv_file_open() finds the protocol
> in filename (probes the file if missing), and then opens with that
> protocol.
> 
>>> Examples:
>>>
>>> -drive file=foo.qcow2,format=qcow2
>>>
>>>  Format "qcow2", protocol "file" with argument filename "foo.img"
>>
>> Actually the protocol is guessed here. For this, not all protocols are
>> considered, it's only between file/host_device/host_cdrom/host_floppy
>> (these are the protocols implementing bdrv_probe_device, and file as the
>> default if no other protocol feels responsible)
> 
> Then we need a way to ask for this guessing, say (pseudo-)protocol
> "auto".

I agree.

>>> -drive file=nbd:unix:/tmp/my_socket,format=raw
>>>
>>>  Format "raw", protocol "nbd" with arguments domain "unix", filename
>>>  "/tmp/my_socket"
>>>
>>> -drive blkdebug:/tmp/blkdebug.cfg:fat:floppy:rw:/tmp/dir
>>>
>>>  Format not specified (system guesses one), protocol "blkdebug" with
>>>  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
>>>  arguments floppy true, dirname "/tmp/dir"
>>
>> These look right to me.
>>
>>>
>>> You see that -drive has a separate option for format, but has protocols
>>> encoded in option file, in their own mini-language.  Doesn't work for
>>> arbitrary filenames.  Besides, mini-languages to encode options in
>>> strings are quite inappropriate for QMP.
>>>
>>> So we need something cleaner for QMP.  Here's a sketch.  Instead of
>>>
>>> - "file": the disk image file to use (json-string, optional)
>>> - "format": disk format (json-string, optional)
>>>     - Possible values: "raw", "qcow2", ...
>>>
>>> have
>>>
>>> - "format": disk format (json-string, optional)
>>>     - Possible values: "raw", "qcow2", ...
>>> - "protocol": json-array of json-object
>>>   Each element object has a member "name"
>>>     - Possible values: "file", "nbd", ...
>>>   Additional members depend on the value of "name".
>>>   For "name" = "file":
>>>     - "file": file name (json-string)
>>>   For "name" = "nbd":
>>>     - "domain": address family (json-string, optional)
>>>         - Possible values: "inet" (default), "unix"
>>>     - "file": file name (json-string), only with "domain" = "unix"
>>>     - "host": host name (json-string), only with "domain" = "inet"
>>>     - "port": port (json-int), only with "domain" = "inet"
>>>   ...
>>>
>>> You get the idea.
>>>
>>> Comments?
>>
>> Makes sense.
>>
>> So blkdebug would define a field "protocol" (json-object) that it uses
>> to initialize the underlying protocol and we would get the stacking this
>> way?
> 
> No, my proposal represents the stack of protocols as json-array, not by
> nesting them.  I should have given examples.  Reusing my three examples
> from above:

Ah, sorry, I missed the part with the array. That's probably even better.

> 
> * Format "qcow2", protocol "auto" with argument filename "foo.img"
> 
>     "format": "qcow2",
>     "protocol": [{ "name": "auto", "file": "foo.qcow2" }],
> 
> * Format "raw", protocol "nbd" with arguments domain "unix", filename
>   "/tmp/my_socket"
> 
>     "format": "raw"
>     "protocol": [{ "name": "nbd", "domain": "unix", "file": "/tmp/my_socket" }]
> 
> * Format not specified (system guesses one), protocol "blkdebug" with
>   argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
>   arguments floppy true, dirname "/tmp/dir"
> 
>     "protocol": [{ "name": "blkdebug", "file": "/tmp/blkdebug.cfg" },
>                  { "name": "fat", "floppy": true, "dir": "/tmp/dir" }]
> 
> With nesting, we'd get something like this instead:
> 
> * Format "qcow2", protocol "auto" with argument filename "foo.img"
> 
>     "format": "qcow2",
>     "protocol": { "name": "auto", "file": "foo.qcow2" }
> 
> * Format "raw", protocol "nbd" with arguments domain "unix", filename
>   "/tmp/my_socket"
> 
>     "format": "raw"
>     "protocol": { "name": "nbd", "domain": "unix", "file": "/tmp/my_socket" }
> 
> * Format not specified (system guesses one), protocol "blkdebug" with
>   argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
>   arguments floppy true, dirname "/tmp/dir"
> 
>     "protocol": { "name": "blkdebug", "file": "/tmp/blkdebug.cfg",
>         "protocol": { "name": "fat", "floppy": true, "dir": "/tmp/dir" }
>     }
> 
> Nesting has one small advantage, namely a protocol's property "stacks on
> top of another protocol" becomes explicit in syntax: it's true iff it
> has a "protocol" member.

Right. The array has a different nice advantage though: Stacked protocol
drivers like blkdebug currently need to call bdrv_file_open manually to
get their underlying protocol. With the array we can do it in generic
code like for formats.

Kevin

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Avi Kivity]

On 06/04/2010 05:16 PM, Markus Armbruster wrote:
> - "protocol": json-array of json-object
>    Each element object has a member "name"
>      - Possible values: "file", "nbd", ...
>    Additional members depend on the value of "name".
>    For "name" = "file":
>      - "file": file name (json-string)
>    For "name" = "nbd":
>      - "domain": address family (json-string, optional)
>          - Possible values: "inet" (default), "unix"
>      - "file": file name (json-string), only with "domain" = "unix"
>      - "host": host name (json-string), only with "domain" = "inet"
>      - "port": port (json-int), only with "domain" = "inet"
>    ...
>
>    

This loses the nesting that protocols have.  I'd like to see the each 
nested protocol as member of the parent protocol.  Besides the lovely } 
} }s in the json representation, this allows us to have more complicated 
protocols, for example a mirror protocol that has two child protocol 
each specifying a different backing store.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: RFC: blockdev_add & friends, brief rationale, QMP docs

[Markus Armbruster]

Avi Kivity <avi@redhat.com> writes:

> On 06/04/2010 05:16 PM, Markus Armbruster wrote:
>> - "protocol": json-array of json-object
>>    Each element object has a member "name"
>>      - Possible values: "file", "nbd", ...
>>    Additional members depend on the value of "name".
>>    For "name" = "file":
>>      - "file": file name (json-string)
>>    For "name" = "nbd":
>>      - "domain": address family (json-string, optional)
>>          - Possible values: "inet" (default), "unix"
>>      - "file": file name (json-string), only with "domain" = "unix"
>>      - "host": host name (json-string), only with "domain" = "inet"
>>      - "port": port (json-int), only with "domain" = "inet"
>>    ...
>>
>>    
>
> This loses the nesting that protocols have.  I'd like to see the each
> nested protocol as member of the parent protocol.  Besides the lovely
> } } }s in the json representation, this allows us to have more
> complicated protocols, for example a mirror protocol that has two
> child protocol each specifying a different backing store.

Even though we don't have such a protocol yet, not even plans to get it,
your argument tips the balance towards nesting.

Revised sketch: instead of

- "file": the disk image file to use (json-string, optional)
- "format": disk format (json-string, optional)
    - Possible values: "raw", "qcow2", ...

have

- "format": image format (json-string, optional)
    - Possible values: "raw", "qcow2", [...]
- "protocol": image access protocol (json-object)
    - It has a member "name" (json-string), and depending on its value
      additional members.
    - For "name" = "auto", "file", [...]
      - "file": name of image file (json-string)
    - For "name" = "nbd":
      - "domain": address family (json-string, optional)
          - Possible values: "inet" (default), "unix"
      - "file": name of socket file (json-string), only with "domain" =
        "unix"
      - "host": host name (json-string), only with "domain" = "inet"
      - "port": port (json-int), only with "domain" = "inet"
    - For "name" = "blkdebug":
      - "config": name of config file (json-string)
      - "protocol": image access protocol (json-object), as above
    [...]

Examples:

* Format "qcow2", protocol "auto" with argument filename "foo.img"

    "format": "qcow2",
    "protocol": { "name": "auto", "file": "foo.qcow2" }

* Format "raw", protocol "nbd" with arguments domain "unix", filename
  "/tmp/my_socket"

    "format": "raw"
    "protocol": { "name": "nbd", "domain": "unix", "file": "/tmp/my_socket" }

* Format not specified (system guesses one), protocol "blkdebug" with
  argument filename "/tmp/blkdebug.cfg" stacked onto protocol "fat" with
  arguments floppy true, dirname "/tmp/dir"

    "protocol": { "name": "blkdebug", "file": "/tmp/blkdebug.cfg",
        "protocol": { "name": "fat", "floppy": true, "dir": "/tmp/dir" }
    }

This nesting business is easy enough for QMP, but it'll be awkward on
the command line.  Ideas on how to do it cleanly with -blockdev?