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;
> > +}
> > +
>
