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~
