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

[Bart Van Assche <bvanassche@xxxxxxx>]

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.