Re: [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands

[Logan Gunthorpe]

Thanks for the Review!

On 2020-02-26 4:28 p.m., Sagi Grimberg wrote:
> This looks questionable... There are tons of features that doesn't
> make sense here like hmb, temperature stuff, irq stuff, timestamps,
> reservations etc... passing-through these will have confusing
> semantics.. Maybe white-list what actually makes sense to passthru?

Yes, I agree a white-list here probably makes sense. I'll try to come up
with a list of features to start that whitelist, though my list might be
a bit different from yours: I don't see why temperature or timestamps
can't be passed through.

Also note: Christoph was advocating against the whitelist for the
commands, though, I agree with you that it is the most sensible approach.

>> +ÂÂÂÂÂÂÂ break;
>> +ÂÂÂ case nvme_admin_identify:
>> +ÂÂÂÂÂÂÂ switch (req->cmd->identify.cns) {
>> +ÂÂÂÂÂÂÂ case NVME_ID_CNS_CTRL:
>> +ÂÂÂÂÂÂÂÂÂÂÂ req->execute = nvmet_passthru_execute_cmd;
>> +ÂÂÂÂÂÂÂÂÂÂÂ req->p.end_req = nvmet_passthru_override_id_ctrl;
>> +ÂÂÂÂÂÂÂÂÂÂÂ return NVME_SC_SUCCESS;
>> +ÂÂÂÂÂÂÂ case NVME_ID_CNS_NS:
>> +ÂÂÂÂÂÂÂÂÂÂÂ req->execute = nvmet_passthru_execute_cmd;
>> +ÂÂÂÂÂÂÂÂÂÂÂ req->p.end_req = nvmet_passthru_override_id_ns;
>> +ÂÂÂÂÂÂÂÂÂÂÂ return NVME_SC_SUCCESS;
> 
> Aren't you missing NVME_ID_CNS_NS_DESC_LIST? and
> NVME_ID_CNS_NS_ACTIVE_LIST?

Well no, seeing they can be passed through the default path.... But in
the light of the comment below, yes.

>> +ÂÂÂÂÂÂÂ default:
>> +ÂÂÂÂÂÂÂÂÂÂÂ return nvmet_setup_passthru_command(req);
>> +ÂÂÂÂÂÂÂ }
> 
> Also here, all the namespace management stuff has questionable
> semantics in my mind...

Yes, I agree with that. I'll make the change in the next revision.

Logan