Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3148 bytes --]

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 <mreitz@redhat.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]