On 6/26/20 1:19 PM, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2020 at 08:48:25PM +0200, Philippe Mathieu-Daudé wrote:
>> We allocate an unique chunk of memory then use it for two
>> different structures. Introduce the 'idsz_max' variable to
>> hold the maximum size, to make it clearer the size is enough
>> to hold the two structures.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> ---
>> FIXME: reword with something that makes more sense...
>> ---
>> block/nvme.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 71f8cf27a8..ffda804a8e 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -438,6 +438,7 @@ static void nvme_identify(BlockDriverState *bs, int
>> namespace, Error **errp)
>> BDRVNVMeState *s = bs->opaque;
>> NvmeIdCtrl *idctrl;
>> NvmeIdNs *idns;
>> + size_t idsz_max;
>> NvmeLBAF *lbaf;
>> uint8_t *resp;
>> uint16_t oncs;
>> @@ -448,14 +449,15 @@ static void nvme_identify(BlockDriverState *bs, int
>> namespace, Error **errp)
>> .cdw10 = cpu_to_le32(0x1),
>> };
>>
>> - resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
>> + idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
>> + resp = qemu_try_blockalign0(bs, idsz_max);
>> if (!resp) {
>> error_setg(errp, "Cannot allocate buffer for identify response");
>> goto out;
>> }
>> idctrl = (NvmeIdCtrl *)resp;
>> idns = (NvmeIdNs *)resp;
>> - r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
>> + r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);
>
> _nvme_check_size() has compile-time asserts that check
> sizeof(NvmeIdCtrl) == sizeof(NvmeIdNs) == 4096.
>
> I suggest the following cleanup:
>
> union {
> NvmeIdCtrl ctrl;
> NvmeIdNs ns;
> } *id;
> ...
> id = qemu_try_blockalign0(bs, sizeof(*id));
> ...
> r = qemu_vfio_dma_map(s->vfio, resp, sizeof(*id), true, &iova);
>
> and accesses to idctl are replaced with id->ctrl and idns with id->ns.
>
> This eliminates the casts, makes it clear that this data is overlapping,
> and avoids the need for idsz_max.
Clever idea, thanks!
