Re: [PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries

[Bart Van Assche <bvanassche@acm.org>]

On 9/15/22 03:34, John Garry wrote:
> On 14/09/2022 23:56, Bart Van Assche wrote:
>> -int scsi_proc_hostdir_add(struct scsi_host_template *sht)
>> +int scsi_proc_hostdir_add(const struct scsi_host_template *sht)
>>   {
>> -    int ret = 0;
>> +    struct scsi_proc_entry *e;
>> +    int ret = -ENOMEM;
>>       if (!sht->show_info)
>>           return 0;
>>       mutex_lock(&global_host_template_mutex);
>> -    if (!sht->present++) {
>> -        sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>> -            if (!sht->proc_dir) {
>> -            printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
>> -                   __func__, sht->proc_name);
>> -            ret = -ENOMEM;
>> -        }
>> +    e = __scsi_lookup_proc_entry(sht);
>> +    if (!e) {
>> +        e = kzalloc(sizeof(*e), GFP_KERNEL);
>> +        if (!e)
>> +            goto unlock;
> 
> maybe it's better to set ret = -ENOMEM here (and not initialize ret), as every other path it is set, AFAICS

I will make this change and also set ret if proc_mkdir() fails.

>>   void scsi_proc_host_add(struct Scsi_Host *shost)
>>   {
>> -    struct scsi_host_template *sht = shost->hostt;
>> +    const struct scsi_host_template *sht = shost->hostt;
>> +    struct scsi_proc_entry *e;
>>       struct proc_dir_entry *p;
>>       char name[10];
>> -    if (!sht->proc_dir)
>> +    if (!sht->show_info)
>> +        return;
>> +
>> +    e = scsi_lookup_proc_entry(sht);
> 
> hmm... this really should not fail, right?. Maybe an error message would be appropiate here (for failure).

If scsi_proc_hostdir_add() failed scsi_lookup_proc_entry() will return NULL.
I will add an error message.

>> +    if (!e)
>>           return;
>>       sprintf(name,"%d", shost->host_no);
>> -    p = proc_create_data(name, S_IRUGO | S_IWUSR,
>> -        sht->proc_dir, &proc_scsi_ops, shost);
>> +    p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
>> +                 &proc_scsi_ops, shost);
>>       if (!p)
>>           printk(KERN_ERR "%s: Failed to register host %d in"
>>                  "%s\n", __func__, shost->host_no,
>> @@ -175,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
>>    */
>>   void scsi_proc_host_rm(struct Scsi_Host *shost)
>>   {
>> +    const struct scsi_host_template *sht = shost->hostt;
>> +    struct scsi_proc_entry *e;
>>       char name[10];
>> -    if (!shost->hostt->proc_dir)
>> +    if (!sht->show_info)
>> +        return;
>> +
>> +    e = scsi_lookup_proc_entry(sht);
> 
> Same comment as scsi_proc_host_add

If scsi_proc_hostdir_add() failed scsi_lookup_proc_entry() will return NULL.

Thanks,

Bart.