Re: [PATCH libnvme 2/1] nvme: Add generic connect parameter support detection

[Sagi Grimberg <sagi@grimberg.me>]

>> --- a/src/libnvme.map
>> +++ b/src/libnvme.map
>> @@ -352,6 +352,7 @@ LIBNVME_1_0 {
>>   		nvmf_treq_str;
>>   		nvmf_trtype_str;
>>   		nvmf_update_config;
>> +		nvmf_check_param_support;
> 
> New entries go to the 'unreleased' section, this would be LIBNVME_1_2.

OK.

>> +int nvmf_check_param_support(const char *param, bool *supported)
>> +{
> 
> I was wondering if t would be better to return all supported features in
> one go. Not really sure about it though.

Why? it is already held in the kernel, why hold it again?

>> +	int ret = 0, fd, len;
>> +	char buf[0x1000];
>> +
>> +	fd = open(nvmf_dev, O_RDWR);
>> +	if (fd < 0) {
>> +		nvme_msg(NULL, LOG_ERR, "Failed to open %s: %s\n",
>> +			 nvmf_dev, strerror(errno));
> 
> 'nvme_msg(NULL,' will always write to the stderr which is kind of bad
> for libraries. I think it would okay just to return the error codes in
> this function and have the caller do the logging part.

OK

> 
>> +		return -ENVME_CONNECT_OPEN;
>> +	}
>> +
>> +	memset(buf, 0x0, sizeof(buf));
>> +	len = read(fd, buf, sizeof(buf) - 1);
>> +	if (len < 0) {
>> +		nvme_msg(NULL, LOG_ERR, "Failed to read from %s: %s\n",
>> +			 nvmf_dev, strerror(errno));
>> +		ret = -ENVME_CONNECT_READ;
>> +		goto out_close;
>> +	}
>> +	nvme_msg(NULL, LOG_DEBUG, "supported opts: '%.*s'\n",
>> (int)strcspn(buf, "\n"), buf);
> 
> This triggered my question above why we don't return all supported
> features up and have the caller added something like this?

Why? the caller doesn't need the entire array, it is just looking
to check if a param is supported. We can move this out if someone
actually needs the entire string or set of caps...