[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encr
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename() |
Date: |
Wed, 2 Sep 2015 16:53:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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 <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.
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 = 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.
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, 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.
>
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename(),
Max Reitz <=