On Sat, Mar 28, 2020 at 6:41 PM Bart Van Assche [off-list ref] wrote:

On 2020-03-20 05:16, Jack Wang wrote:

quoted

+static int __read_mostly port_nr = RTRS_PORT;

Would uint16_t be sufficient for this kernel module parameter?

yes, u16 is enough.

Is this kernel module parameter used anywhere in the hot path? If not,
should __read_mostly perhaps be left out?

not in hot path, __read_mostly can be removed.

quoted

+module_param_named(port_nr, port_nr, int, 0444);
+MODULE_PARM_DESC(port_nr,
+              "The port number server is listening on (default: "

                                ^^^
                                the?

right!

quoted

+              __stringify(RTRS_PORT)")");
+
+#define DEFAULT_DEV_SEARCH_PATH "/"

quoted

+static void destroy_device(struct rnbd_srv_dev *dev)
+{
+     WARN(!list_empty(&dev->sess_dev_list),
+          "Device %s is being destroyed but still in use!\n",
+          dev->id);

Has it been considered to change WARN() into WARN_ONCE()?

ok.

quoted

+static int rnbd_srv_rdma_ev(struct rtrs_srv *rtrs, void *priv,
+                          struct rtrs_srv_op *id, int dir,
+                          void *data, size_t datalen, const void *usr,
+                          size_t usrlen)
+{
+     struct rnbd_srv_session *srv_sess = priv;
+     const struct rnbd_msg_hdr *hdr = usr;
+     int ret = 0;
+     u16 type;
+
+     if (WARN_ON(!srv_sess))
+             return -ENODEV;

Same question here: how about using WARN_ON_ONCE() instead of WARN_ON()?

ok.

quoted

+static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
+                                  const char *dev_name)
+{
+     char *full_path;
+     char *a, *b;
+
+     full_path = kmalloc(PATH_MAX, GFP_KERNEL);
+     if (!full_path)
+             return ERR_PTR(-ENOMEM);
+
+     /*
+      * Replace %SESSNAME% with a real session name in order to
+      * create device namespace.
+      */
+     a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
+     if (a) {
+             int len = a - dev_search_path;
+
+             len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
+                            dev_search_path, srv_sess->sessname, dev_name);
+             if (len >= PATH_MAX) {
+                     pr_err("Tooooo looong path: %s, %s, %s\n",
+                            dev_search_path, srv_sess->sessname, dev_name);
+                     kfree(full_path);
+                     return ERR_PTR(-EINVAL);
+             }
+     } else {
+             snprintf(full_path, PATH_MAX, "%s/%s",
+                      dev_search_path, dev_name);
+     }

Has it been considered to use kasprintf() instead of kmalloc() + snprintf()?

will convert to kasprintf.

Otherwise this patch looks fine to me.

Thanks,

Bart.

Thanks Bart!