From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47611) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWW1y-0000Bi-Un for qemu-devel@nongnu.org; Mon, 31 Aug 2015 16:50:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWW1x-0000ox-U5 for qemu-devel@nongnu.org; Mon, 31 Aug 2015 16:50:18 -0400 References: <1439939415-18180-1-git-send-email-mreitz@redhat.com> <1439939415-18180-2-git-send-email-mreitz@redhat.com> From: Eric Blake Message-ID: <55E4BDFF.70002@redhat.com> Date: Mon, 31 Aug 2015 14:50:07 -0600 MIME-Version: 1.0 In-Reply-To: <1439939415-18180-2-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Tn3O92gTO5WdtH5ddc7E0xe8fwqom4Boo" Subject: Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Tn3O92gTO5WdtH5ddc7E0xe8fwqom4Boo Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08/18/2015 05:10 PM, Max Reitz wrote: > Instead of returning a pointer to the filename, copy it into a buffer > specified by the caller. >=20 > Signed-off-by: Max Reitz > --- > block.c | 25 ++++++++++++++++++------- > include/block/block.h | 2 +- > monitor.c | 6 +++++- > 3 files changed, 24 insertions(+), 9 deletions(-) Would it be better to just have bdrv_get_encrypted_filename() return a g_malloc'd buffer, instead of making the caller do it? Then the buffer can be g_strdup'd to the correct size, instead of over-allocating PATH_MAX bytes when a smaller size will usually do. >=20 > diff --git a/block.c b/block.c > index d088ee0..41b0f85 100644 > --- a/block.c > +++ b/block.c > @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const c= har *key, Error **errp) > } > } else { > if (bdrv_key_required(bs)) { > + char *enc_filename =3D g_malloc(PATH_MAX); > + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); > error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED, > "'%s' (%s) is encrypted", > bdrv_get_device_or_node_name(bs), > - bdrv_get_encrypted_filename(bs)); > + enc_filename); Should you assert(enc_filename) to prove we know we aren't calling error_set("%s", NULL)? After all, bdrv_get_encrypted_filename() can return NULL, but only if no encrypted name is available; while bdrv_key_required() implies that an encrypted name is available. > -const char *bdrv_get_encrypted_filename(BlockDriverState *bs) > +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, si= ze_t sz) > { > - if (bs->backing_hd && bs->backing_hd->encrypted) > - return bs->backing_file; > - else if (bs->encrypted) > - return bs->filename; > - else > + if (sz > INT_MAX) { > + sz =3D INT_MAX; > + } > + > + if (bs->backing_hd && bs->backing_hd->encrypted) { > + pstrcpy(dest, sz, bs->backing_file); > + return dest; Again, using g_strdup() here instead of making the caller pass in a buffer might be nicer semantics (certainly fewer places that have to pre-allocate g_malloc0(PATH_MAX) bytes). > +++ b/monitor.c > @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, B= lockDriverState *bs, > BlockCompletionFunc *completion_cb, > void *opaque) > { > + char *enc_filename; > int err; > =20 > + enc_filename =3D g_malloc(PATH_MAX); > + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); > monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_nam= e(bs), > - bdrv_get_encrypted_filename(bs)); > + enc_filename); > + g_free(enc_filename); And once again, an assert(enc_filename) might be nice to be sure we aren't dealing with a NULL return. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Tn3O92gTO5WdtH5ddc7E0xe8fwqom4Boo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV5L3/AAoJEKeha0olJ0Nq0UUIAKLrtm5e2eFFi3mGJ5NTWFy9 1emzwheaPJsw/d5pcAsXRB4qcYB6ma0A9HXcTSPucRe6EgpoDhJ6X/XnmXMN0DQJ 5dLgNVuPfrLp5f99o7XDKyUPpiZ9e3/HRt8xJr1FsA+YqEjzDkvoNKEzW+80pTI/ FnwyuNPlZJpaZ1RL70dQvI7MDZ4RW7cZupV1xikfY9e5zjUR+yvEJUgThX50QdMK MPRRvO2UQ40XVaahrwiXJeDmuQJzb4fsrTjVcmeDrWzoj5eehrVHOZ9KucYbmdrF UzEvJPjveJoF3P6vHLWzWJ0drvtT4wlwup5wTbIUDKc5E5AyKNiKHeLH+LR0YR0= =etWH -----END PGP SIGNATURE----- --Tn3O92gTO5WdtH5ddc7E0xe8fwqom4Boo--