[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filenam
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename() |
Date: |
Mon, 31 Aug 2015 14:50:07 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
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 <address@hidden>
> ---
> 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.
>
> 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 = 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, size_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 = 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,
> BlockDriverState *bs,
> BlockCompletionFunc *completion_cb,
> void *opaque)
> {
> + char *enc_filename;
> int err;
>
> + enc_filename = g_malloc(PATH_MAX);
> + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
> monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 0/6] block: Drop BDS.filename, Max Reitz, 2015/08/18
- [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename(), Max Reitz, 2015/08/18
- Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename(),
Eric Blake <=
- [Qemu-devel] [PATCH v4 2/6] block: Avoid BlockDriverState.filename, Max Reitz, 2015/08/18
- [Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename(), Max Reitz, 2015/08/18
- [Qemu-devel] [PATCH v4 4/6] qemu-img: Use bdrv_filename_alloc() for map, Max Reitz, 2015/08/18
- [Qemu-devel] [PATCH v4 6/6] iotests: Test changed Quorum filename, Max Reitz, 2015/08/18
- [Qemu-devel] [PATCH v4 5/6] block: Drop BlockDriverState.filename, Max Reitz, 2015/08/18