On Tue, Oct 22, 2019 at 03:10:26PM -0700, Bart Van Assche wrote:
> On 2019-10-22 00:00, Honggang LI wrote:
> > +static bool use_imm_data(void)
> > +{
> > +#ifdef __linux__
> > + bool ret = false;
> > + char flag = 0;
> > + int cnt;
> > + int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
> > +
> > + if (fd < 0)
> > + return false;
> > + cnt = read(fd, &flag, 1);
> > + if (cnt != 1)
> > + return false;
> > +
> > + if (!strncmp(&flag, "Y", 1))
> > + ret = true;
> > + close(fd);
> > + return ret;
> > +#else
> > + return false;
> > +#endif
> > +}
>
> There is already plenty of Linux-specific code in srp_daemon. The #ifdef
> __linux__ / #endif guard does not seem useful to me.
Will delete the guard.
>
> There is a file descriptor leak in the above function, namely if read()
> returns another value than 1.
Yes, will fix it.
>
> The use_imm_data kernel module parameter was introduced in kernel v5.0
> (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The
> max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8;
> "RDMA/srp: Add parse function for maximum initiator to target IU size").
>
> So the above check will help for kernel versions before v5.0 but not for
> kernel versions [v5.0..v5.5).
Yes, you are right. The patch does not fix the issue for kernel
versions [v5.0..v5.5). But it also does not do anything bad for
kernel versions before v5.5 (commit 547ed331bbe8). It will fix
the issue for kernel after 547ed331bbe8.
> If that is really what you want, please
> explain this in a comment above the use_imm_data() function.
Yes, will add a comment for it.
thanks
