From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dEFg8-0008Tp-Gq for qemu-devel@nongnu.org; Fri, 26 May 2017 09:53:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dEFg7-0004tH-Ju for qemu-devel@nongnu.org; Fri, 26 May 2017 09:53:20 -0400 Date: Fri, 26 May 2017 14:53:09 +0100 From: "Daniel P. Berrange" Message-ID: <20170526135309.GI24484@redhat.com> Reply-To: "Daniel P. Berrange" References: <20170525163851.8047-1-berrange@redhat.com> <20170525163851.8047-10-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v7 09/20] qcow: convert QCow to use QCryptoBlock for encryption List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Eric Blake , Max Reitz , Kevin Wolf On Fri, May 26, 2017 at 02:03:40PM +0200, Alberto Garcia wrote: > On Thu 25 May 2017 06:38:40 PM CEST, Daniel P. Berrange wrote: > > @@ -105,6 +116,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > > int ret; > > QCowHeader header; > > Error *local_err = NULL; > > + QCryptoBlockOpenOptions *crypto_opts = NULL; > > + unsigned int cflags = 0; > > + QDict *encryptopts = NULL; > > + const char *encryptfmt; > > + > > + qdict_extract_subqdict(options, &encryptopts, "encrypt."); > > + encryptfmt = qdict_get_try_str(encryptopts, "format"); > > > > bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, > > false, errp); > > if (!bs->file) { > > return -EINVAL; > > } > > You're leaking encryptopts if the function returns here. > > > @@ -873,6 +850,20 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) > > goto exit; > > } > > header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES); > > + > > + crypto_opts = block_crypto_create_opts_init( > > + Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + ret = -EINVAL; > > + goto exit; > > + } > > Not very important, and my fault for not having pointed it out in my > previous review, but you can spare the error_propagate() call if you > pass errp directly to block_crypto_create_opts_init() and then check if > crypto_opts is NULL. > > Actually none of the error_propagate() calls in qcow_create() is really > necessary, but that could be fixed in a separate patch, if at all (it's > not so important). Since I have to re-post to fix the leak anyway, I'll eliminate the intermedia local_err usage - I like to avoid that when not needed anyway. > > The leak however needs to be fixed. With that, > > Reviewed-by: Alberto Garcia Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|