From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:37846)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1YvWWS-0003Q3-Kb
	for qemu-devel@nongnu.org; Thu, 21 May 2015 15:52:53 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1YvWWO-0004rP-Km
	for qemu-devel@nongnu.org; Thu, 21 May 2015 15:52:52 -0400
Received: from mail-qk0-x22c.google.com ([2607:f8b0:400d:c09::22c]:35816)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1YvWWO-0004rL-GJ
	for qemu-devel@nongnu.org; Thu, 21 May 2015 15:52:48 -0400
Received: by qkdn188 with SMTP id n188so59882665qkd.2
	for <qemu-devel@nongnu.org>; Thu, 21 May 2015 12:52:47 -0700 (PDT)
Sender: Richard Henderson <rth7680@gmail.com>
Message-ID: <555E378B.2070901@twiddle.net>
Date: Thu, 21 May 2015 12:52:43 -0700
From: Richard Henderson <rth@twiddle.net>
MIME-Version: 1.0
References: <1432205817-16414-1-git-send-email-berrange@redhat.com>
	<1432205817-16414-5-git-send-email-berrange@redhat.com>
In-Reply-To: <1432205817-16414-5-git-send-email-berrange@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API
 & built-in implementation
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Daniel P. Berrange" <berrange@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>

On 05/21/2015 03:56 AM, Daniel P. Berrange wrote:
> +QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> +                                  QCryptoCipherMode mode,
> +                                  const uint8_t *key, size_t nkey,
> +                                  Error **errp)
> +{
> +    QCryptoCipher *cipher;
> +
> +    cipher = g_new0(QCryptoCipher, 1);
> +    cipher->alg = alg;
> +    cipher->mode = mode;
> +
> +    switch (cipher->alg) {
> +    case QCRYPTO_CIPHER_ALG_DES_RFB:
> +        if (qcrypto_cipher_init_des_rfb(cipher, key, nkey, errp) < 0) {
> +            goto error;
> +        }
> +        break;
> +    case QCRYPTO_CIPHER_ALG_AES:
> +        if (qcrypto_cipher_init_aes(cipher, key, nkey, errp) < 0) {
> +            goto error;
> +        }
> +        break;
> +    default:
> +        error_setg(errp,
> +                   _("Unsupported cipher algorithm %d"), cipher->alg);
> +        goto error;
> +    }
> +
> +    return cipher;
> +
> + error:
> +    g_free(cipher);
> +    return NULL;
> +}

Is it really that helpful to have all of these switches, as opposed to having
one function per cipher and calling it directly?  Similarly for the hashing.

The uses I pulled out of the later patches are like

+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256,
+                            qiov->iov, qiov->niov,
+                            &data, &len,
+                            NULL) < 0) {
+        return -EINVAL;

+    if (qcrypto_hash_base64(QCRYPTO_HASH_ALG_SHA1,
+                            combined_key,
+                            WS_CLIENT_KEY_LEN + WS_GUID_LEN,
+                            &accept,
+                            &err) < 0) {

+    cipher = qcrypto_cipher_new(
+        QCRYPTO_CIPHER_ALG_DES_RFB,
+        QCRYPTO_CIPHER_MODE_ECB,
+        key, G_N_ELEMENTS(key),
+        &err);

+    s->cipher = qcrypto_cipher_new(
+        QCRYPTO_CIPHER_ALG_AES,
+        QCRYPTO_CIPHER_MODE_CBC,
+        keybuf, G_N_ELEMENTS(keybuf),
+        &err);

This one could have explicitly specified AES128, but you hid that behind
G_N_ELEMENTS.  Which seems like obfuscation to me, but...


r~