Re: [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command

[Max Reitz <mreitz@redhat.com>]

On 2014-12-05 at 14:24, Eric Blake wrote:
> On 12/05/2014 03:08 AM, Max Reitz wrote:
>> Use separate code paths for the two overloaded functions of the 'change'
>> HMP command, and invoke the 'blockdev-change-medium' QMP command if used
>> on a block device (by calling qmp_blockdev_change_medium()).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   hmp.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +    } else {
>> +        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
>> +        if (err &&
>> +            error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
>> +            error_free(err);
>> +            monitor_read_block_device_key(mon, device, NULL, NULL);
>>               return;
> Hmm. Not a problem with this patch (which is just reorganizing control
> flow), but a possible design issue.  The old 'change' QMP command cannot
> provide the encryption code, hence it cannot succeed when changing to an
> encrypted media.  But now that we are adding a new QMP command, we could
> possibly rectify that matter.  However, the way I'm envisioning that is
> that blockdev-change-medium gains an optional parameter for encryption
> key.  Then the HMP command gains a new optional parameter for specifying
> the key, and the logic flow would look a bit more like:
>
> qmp_blockdev_change_medium(device, target, !!arg, arg,
>                             !!key, key, &err);
> if (err && error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED &&
>      !key) {
>      error_free(err);
>      key = prompt_for_key_from_monitor;
>      qmp_blockdev_change_medium(device, target, !!arg, arg,
>                                 true, key, &err);
> }
>
> which would mean that a QMP command can now supply the key in one
> command, rather than HMP being the only way to supply a password because
> of how it does a two-step process (that is,
> monitor_read_block_device_key isn't really accessible from QMP).

I'd rather put all of the encryption stuff back until we have figured 
out how to fix it (that is, if we want to fix it at all)...

Max