Re: [Qemu-devel] [PATCH v2 1/5] block: Change bdrv_get_encrypted_filename()

[Max Reitz <mreitz@redhat.com>]

On 06.08.2015 04:01, Wen Congyang wrote:
> On 08/06/2015 04:52 AM, 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               | 24 +++++++++++++++++-------
>>   include/block/block.h |  2 +-
>>   monitor.c             |  4 +++-
>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index d088ee0..e7dd2f1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2760,10 +2760,12 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
>>           }
>>       } else {
>>           if (bdrv_key_required(bs)) {
>> +            char enc_filename[PATH_MAX];
>
> I think it's better to use g_malloc() here.
>
>> +            bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
>>               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);
>>           }
>>       }
>>   }
>> @@ -2980,14 +2982,22 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
>>       return false;
>>   }
>>
>> -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;
>> +    } else if (bs->encrypted) {
>> +        pstrcpy(dest, sz, bs->filename);
>> +        return dest;
>> +    } else {
>> +        dest[0] = '\0';
>>           return NULL;
>> +    }
>>   }
>>
>>   void bdrv_get_backing_filename(BlockDriverState *bs,
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 37916f7..a78e4f1 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -430,7 +430,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
>>                               int64_t *cluster_sector_num,
>>                               int *cluster_nb_sectors);
>>
>> -const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>> +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz);
>>   void bdrv_get_backing_filename(BlockDriverState *bs,
>>                                  char *filename, int filename_size);
>>   void bdrv_get_full_backing_filename(BlockDriverState *bs,
>> diff --git a/monitor.c b/monitor.c
>> index aeea2b5..cfdf781 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5292,10 +5292,12 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
>>                                   BlockCompletionFunc *completion_cb,
>>                                   void *opaque)
>>   {
>> +    char enc_filename[PATH_MAX];
>
> same too.

You're right. I'll change it (in every patch of this series that pattern 
appears in).

Thanks for reviewing!

Max

>
> Thanks
> Wen Congyang
>
>>       int err;
>>
>> +    bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
>>       monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
>> -                   bdrv_get_encrypted_filename(bs));
>> +                   enc_filename);
>>
>>       mon->password_completion_cb = completion_cb;
>>       mon->password_opaque = opaque;
>>
>