On Fri, May 29, 2015 at 10:39:08AM +0800, Gonglei wrote: > On 2015/5/22 17:10, Daniel P. Berrange wrote: > > On Thu, May 21, 2015 at 12:52:43PM -0700, Richard Henderson wrote: > >> 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. > > > > These switches are just an artifact of this default built-in implementation > > where we're jumping off to one or our two built-in crypto algorithsm. The > > gcrypt backend of these APIs has no such switch, since there is just a > > similar looking gcrypt API we directly pass through to. > > > > Similarly, if we add a backend that delegates to the Linux kernel crypto > > API, then we'd just be doing a more or less straight passthrough with none > > of these switches. > > > >> > >> 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... > > > > In designing the APIs I was looking forward to uses beyond those shown > > in this current patch series. In particular with full disk encryption > > there will be a wide selection of algorithms that can be used with the > > implementation, so the caller of the APIs will not be passing in a > > fixed algorithm constant, but instead have it vary according to the > > data format. So on balance I think this current design is more future > > proof than what you suggest > > > > Form your code, we can see that exists many duplicate code about encryption > and > decryption, which have the same arguments, such as qcrypto_cipher_encrypt() > and qcrypto_cipher_decrypt(). Can we just use an argument to check the > operation > is encryption or decryption, then invoke corresponding functions? In this > way, it will decrease lots of duplicate code. IIRC OpenSSL EVP api do this > work > using this way.
What you describe was done with the qcow2_encrypt_sectors() method, where it accepts an 'int enc' parameter to indicate whether it does encryption or decryption. In looking at the qcow2 code for encryption this is one of the things that I found rather unhelpful, as it makes it less obvious to the casual reader what is going on. Having the explicitly _encrypt() and _decrypt() APIs makes the code clearer to follow IMHO, and I don't think size difference in code is appreciable enough to counteract this benefit. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|