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 <address@hidden> 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
> >
> >    
>