Re: [PATCH v2 12/12] target: add virtual remote target

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Fri, Mar 10, 2023 at 04:32:32PM -0600, Mike Christie wrote:
> 
> Just some nits.
> 
> On 3/7/23 2:07 AM, Dmitry Bogdanov wrote:
> > +
> > +static int tcm_remote_port_link(
> > +     struct se_portal_group *se_tpg,
> > +     struct se_lun *lun)
> > +{
> > +     pr_debug("TCM_Remote_ConfigFS: Port Link LUN %lld Successful\n",
> > +               lun->unpacked_lun);
> 
> The l in lun should be one space to the left so it's under the ".
> It will then match the other code and checkpatch won't complain.
> 
> 
> > +     return 0;
> > +}
> > +
> > +static void tcm_remote_port_unlink(
> > +     struct se_portal_group *se_tpg,
> > +     struct se_lun *lun)
> > +{
> > +     pr_debug("TCM_Remote_ConfigFS: Port Unlink LUN %lld Successful\n",
> > +               lun->unpacked_lun);
> 
> Same as above.
> 
> > +}
> > +
> > +static struct se_portal_group *tcm_remote_make_tpg(
> > +     struct se_wwn *wwn,
> > +     const char *name)
> > +{
> > +     struct tcm_remote_hba *remote_hba = container_of(wwn,
> > +                     struct tcm_remote_hba, remote_hba_wwn);
> > +     struct tcm_remote_tpg *remote_tpg;
> > +     unsigned long tpgt;
> > +     int ret;
> > +
> > +     if (strstr(name, "tpgt_") != name) {
> > +             pr_err("Unable to locate \"tpgt_#\" directory group\n");
> > +             return ERR_PTR(-EINVAL);
> > +     }
> > +     if (kstrtoul(name+5, 10, &tpgt))
> 
> Add spaces so it looks like: "name + 5"
> 
> 
> 
> > +}
> > +
> > +static ssize_t tcm_remote_wwn_version_show(struct config_item *item, char *page)
> > +{
> > +     return sprintf(page, "TCM Remote Fabric module %s\n", TCM_REMOTE_VERSION);
> > +}
> 
> 
> Do we need this? I saw other LIO drivers like iscsi, fcoe and loop have them
> but they are never updated so it seems useless.
> 
> For something like stable trying to manage versions is going to be difficult
> as well, so it might just be more confusing.
> 

I absolutely agree that in-kernel modules should not have its own
versions. Kernel version is enough. 

But targetcli tool reads that file for a fabric and I don't want to
work it around when I will add support of remote fabric to it.
So, I am going to keep the version attribute.

> > +
> > +static int __init tcm_remote_fabric_init(void)
> > +{
> > +     int ret = -ENOMEM;
> 
> You can drop the -ENOMEM since we set ret the next line.

I can drop ret variable here at all.

> > +
> > +     ret = target_register_template(&remote_ops);
> > +     if (ret)
> > +             goto out;
> > +
> > +     return 0;
> > +
> > +out:
> > +     return ret;
> > +}
> > +
>