From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49952) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZX9Pe-0003Q1-Pw for qemu-devel@nongnu.org; Wed, 02 Sep 2015 10:53:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZX9Pd-000455-I4 for qemu-devel@nongnu.org; Wed, 02 Sep 2015 10:53:22 -0400 References: <1439939415-18180-1-git-send-email-mreitz@redhat.com> <1439939415-18180-2-git-send-email-mreitz@redhat.com> <55E4BDFF.70002@redhat.com> From: Max Reitz Message-ID: <55E70D58.4050406@redhat.com> Date: Wed, 2 Sep 2015 16:53:12 +0200 MIME-Version: 1.0 In-Reply-To: <55E4BDFF.70002@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Gx1XFtnVCQSV40o85bmfNsxkAIbV7e86b" 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: Eric Blake , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Gx1XFtnVCQSV40o85bmfNsxkAIbV7e86b Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 31.08.2015 22:50, Eric Blake wrote: > 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. >> >> Signed-off-by: Max Reitz >> --- >> block.c | 25 ++++++++++++++++++------- >> include/block/block.h | 2 +- >> monitor.c | 6 +++++- >> 3 files changed, 24 insertions(+), 9 deletions(-) >=20 > 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. Probably, yes. I wanted the interface to be similar to the new bdrv_filename(), but since all the callers of bdrv_get_encrypted_filename() would have to allocate the buffer anyway, I probably should pass through bdrv_filename_alloc() (and more on that in my reply to your review of patch 3). >> >> 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 = char *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); >=20 > 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. Well, in this version, enc_filename will never be NULL because it has been returned by g_malloc(). But it may become necessary with the interface change you suggested (bdrv_get_encrypted_filename() allocating the buffer and returning it), so I'll keep it in mind. Thanks for your review and your suggestions! :-) Max >> -const char *bdrv_get_encrypted_filename(BlockDriverState *bs) >> +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, s= ize_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; >=20 > 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). >=20 >=20 >> +++ b/monitor.c >> @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, = BlockDriverState *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_na= me(bs), >> - bdrv_get_encrypted_filename(bs)); >> + enc_filename); >> + g_free(enc_filename); >=20 > And once again, an assert(enc_filename) might be nice to be sure we > aren't dealing with a NULL return. >=20 --Gx1XFtnVCQSV40o85bmfNsxkAIbV7e86b 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 iQEcBAEBCAAGBQJV5w1YAAoJEDuxQgLoOKytKEgH/i/l5lCFzOdkGXRmzmFcyp+F ewr3SNz3GP2QXQrPs9UxOOWndepxdDpmBYUo4ZxFKcehguXJxbvcBI8v8NTV9RS3 I/siX9ZZus0FTiAmoJneesbvmOE9zFT1AC6C/wNI5bi1gx+qxQ5sVCYJFJQRZc/Q 1k5zLO5IvLIgXJxPFA1IwUJprCOo0OPOnrBOMTsxAQHbGL98Td7pnBh327HKBVuH t7XOWIlH3Kitm9aiR6Mz82g3ZoYjOMHzRmmkMejLXcfheyf9+AHn5zPpRZPD40LL 6pB4QWYZOtpML1vNmcp+jXVRj3CbHWDLcKz+U/kWO4aWIhBAVPAo5d8TGvQrmwI= =0epe -----END PGP SIGNATURE----- --Gx1XFtnVCQSV40o85bmfNsxkAIbV7e86b--