From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwsvY-00025k-TS for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:28:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XwsvP-0007EU-Ty for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:28:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38362) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwsvP-0007EK-M2 for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:27:59 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB5DRxl4003423 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 5 Dec 2014 08:27:59 -0500 Message-ID: <5481B2DC.40402@redhat.com> Date: Fri, 05 Dec 2014 14:27:56 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417774136-30001-1-git-send-email-mreitz@redhat.com> <1417774136-30001-3-git-send-email-mreitz@redhat.com> <5481B205.5040301@redhat.com> In-Reply-To: <5481B205.5040301@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Stefan Hajnoczi , Luiz Capitulino 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 >> --- >> hmp.c | 27 +++++++++++++++------------ >> 1 file changed, 15 insertions(+), 12 deletions(-) > Reviewed-by: Eric Blake > >> + } 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