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

[Max Reitz <mreitz@redhat.com>]

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

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 <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.

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.
> 



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