Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/3/20 11:22 AM, Klaus Jensen wrote:
> On Jul  3 11:14, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 10:37 AM, Klaus Jensen wrote:
>>> On Jul  3 10:20, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>>>> Active Namespace ID list.
>>>>
>>>> Can we have a definition instead of this 0xfffffffe magic value please?
>>>>
>>>
>>> Hmm, not really actually. It's not a magic value, its just because the
>>> logic in Active Namespace ID list would require that it should report
>>> any namespaces with ids *higher* than the one specified, so since
>>> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
>>> needs to be as well.
>>
>> OK.
>>
>>>
>>> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
>>> The original condition just reads well if you are sitting with the spec
>>> on the side.
>>
>> IMO this is clearer:
>>
>>   if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
>>       return NVME_INVALID_NSID | NVME_DNR;
>>   }
>>
> 
> But since min_nsid is uint32_t that would not be wise ;)

Hmm indeed.

> 
> I'll go with the - 1 and add a comment!

Good, thanks.

> 
>> Whichever form you prefer you can amend to the respin patch:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>>
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>>  hw/block/nvme.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>>>  
>>>>>      trace_pci_nvme_identify_nslist(min_nsid);
>>>>>  
>>>>> +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>>>> +        return NVME_INVALID_NSID | NVME_DNR;
>>>>> +    }
>>>>> +
>>>>>      list = g_malloc0(data_len);
>>>>>      for (i = 0; i < n->num_namespaces; i++) {
>>>>>          if (i < min_nsid) {
>>>>>
>>>>
>>>
>>
>