LU-6142 lnet: code style cleanups, improve error handling

During submission of the dynamic LNet work to the upstream
client Dan Carpenter suggested several improvements and a
few fixes. They are:

1) Rename the goto labels to be more clear.

2) Replace CLASSERT with BUILD_BUG_ON

3) Rework the loops to reduce the code indentation level
   and to make them more clear.

4) Add test to ensure the ioctl data is really smaller
   than what is reported in the ioctl header.

5) Fix one off error in lnet_get_peer_info

6) Handle errors instead of sucesses in lnet_dyn_[un]configure

Change-Id: If003d313ae587fd6311d9dfb419e8c6f59ef4705
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: James Simmons <uja.ornl@yahoo.com>
Reviewed-on: https://review.whamcloud.com/18469
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Doug Oucharek <doug.s.oucharek@intel.com>
Reviewed-by: Olaf Weber <olaf@sgi.com>
Reviewed-by: Dmitry Eremin <dmitry.eremin@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lnet/lnet/api-ni.c b/lnet/lnet/api-ni.c
index a8eed05..f75a507 100644 (file)
--- a/lnet/lnet/api-ni.c
+++ b/lnet/lnet/api-ni.c
@@ -1574,7 +1574,7 @@ LNetNIInit(lnet_pid_t requested_pid)
        /* Add in the loopback network */
        if (lnet_ni_alloc(LNET_MKNET(LOLND, 0), NULL, &net_head) == NULL) {
                rc = -ENOMEM;
-               goto failed0;
+               goto err_empty_list;
        }
 
        /* If LNet is being initialized via DLC it is possible
@@ -1587,44 +1587,45 @@ LNetNIInit(lnet_pid_t requested_pid)
                rc = lnet_parse_networks(&net_head,
                                         lnet_get_networks());
                if (rc < 0)
-                       goto failed0;
+                       goto err_empty_list;
        }
 
        ni_count = lnet_startup_lndnis(&net_head);
        if (ni_count < 0) {
                rc = ni_count;
-               goto failed0;
+               goto err_empty_list;
        }
 
        if (!the_lnet.ln_nis_from_mod_params) {
                rc = lnet_parse_routes(lnet_get_routes(), &im_a_router);
                if (rc != 0)
-                       goto failed1;
+                       goto err_shutdown_lndnis;
 
                rc = lnet_check_routes();
                if (rc != 0)
-                       goto failed2;
+                       goto err_destroy_routes;
 
                rc = lnet_rtrpools_alloc(im_a_router);
                if (rc != 0)
-                       goto failed2;
+                       goto err_destroy_routes;
        }
 
        rc = lnet_acceptor_start();
        if (rc != 0)
-               goto failed2;
+               goto err_destroy_routes;
+
        the_lnet.ln_refcount = 1;
        /* Now I may use my own API functions... */
 
        rc = lnet_ping_info_setup(&pinfo, &md_handle, ni_count, true);
        if (rc != 0)
-               goto failed3;
+               goto err_acceptor_stop;
 
        lnet_ping_target_update(pinfo, md_handle);
 
        rc = lnet_router_checker_start();
        if (rc != 0)
-               goto failed4;
+               goto err_stop_ping;
 
        lnet_fault_init();
        lnet_proc_init();
@@ -1633,22 +1634,23 @@ LNetNIInit(lnet_pid_t requested_pid)
 
        return 0;
 
-failed4:
+err_stop_ping:
        lnet_ping_target_fini();
-failed3:
+err_acceptor_stop:
        the_lnet.ln_refcount = 0;
        lnet_acceptor_stop();
-failed2:
+err_destroy_routes:
        if (!the_lnet.ln_nis_from_mod_params)
                lnet_destroy_routes();
-failed1:
+err_shutdown_lndnis:
        lnet_shutdown_lndnis();
-failed0:
+err_empty_list:
        lnet_unprepare();
        LASSERT(rc < 0);
        mutex_unlock(&the_lnet.ln_api_mutex);
        while (!list_empty(&net_head)) {
                struct lnet_ni *ni;
+
                ni = list_entry(net_head.next, struct lnet_ni, ni_list);
                list_del_init(&ni->ni_list);
                lnet_ni_free(ni);
@@ -1726,17 +1728,16 @@ lnet_fill_ni_info(struct lnet_ni *ni, struct lnet_ioctl_config_data *config)
        if (!net_config)
                return;
 
-       CLASSERT(ARRAY_SIZE(ni->ni_interfaces) ==
-                ARRAY_SIZE(net_config->ni_interfaces));
+       BUILD_BUG_ON(ARRAY_SIZE(ni->ni_interfaces) !=
+                    ARRAY_SIZE(net_config->ni_interfaces));
 
-       if (ni->ni_interfaces[0] != NULL) {
-               for (i = 0; i < ARRAY_SIZE(ni->ni_interfaces); i++) {
-                       if (ni->ni_interfaces[i] != NULL) {
-                               strncpy(net_config->ni_interfaces[i],
-                                       ni->ni_interfaces[i],
-                                       sizeof(net_config->ni_interfaces[i]));
-                       }
-               }
+       for (i = 0; i < ARRAY_SIZE(ni->ni_interfaces); i++) {
+               if (!ni->ni_interfaces[i])
+                       break;
+
+               strncpy(net_config->ni_interfaces[i],
+                       ni->ni_interfaces[i],
+                       sizeof(net_config->ni_interfaces[i]));
        }
 
        config->cfg_nid = ni->ni_nid;
@@ -1747,13 +1748,14 @@ lnet_fill_ni_info(struct lnet_ni *ni, struct lnet_ioctl_config_data *config)
 
        net_config->ni_status = ni->ni_status->ns_status;
 
-       for (i = 0;
-            ni->ni_cpts != NULL && i < ni->ni_ncpts &&
-            i < LNET_MAX_SHOW_NUM_CPT;
-            i++)
-               net_config->ni_cpts[i] = ni->ni_cpts[i];
+       if (ni->ni_cpts) {
+               int num_cpts = min(ni->ni_ncpts, LNET_MAX_SHOW_NUM_CPT);
+
+               for (i = 0; i < num_cpts; i++)
+                       net_config->ni_cpts[i] = ni->ni_cpts[i];
 
-       config->cfg_ncpts = ni->ni_ncpts;
+               config->cfg_ncpts = num_cpts;
+       }
 
        /*
         * See if user land tools sent in a newer and larger version
@@ -1787,7 +1789,7 @@ lnet_get_net_config(struct lnet_ioctl_config_data *config)
        struct list_head *tmp;
        int idx = config->cfg_count;
        int rc = -ENOENT;
-       int cpt;
+       int cpt, i = 0;
 
        if (unlikely(!config->cfg_bulk))
                return -EINVAL;
@@ -1795,14 +1797,15 @@ lnet_get_net_config(struct lnet_ioctl_config_data *config)
        cpt = lnet_net_lock_current();
 
        list_for_each(tmp, &the_lnet.ln_nis) {
+               if (i++ != idx)
+                       continue;
+
                ni = list_entry(tmp, lnet_ni_t, ni_list);
-               if (idx-- == 0) {
-                       rc = 0;
-                       lnet_ni_lock(ni);
-                       lnet_fill_ni_info(ni, config);
-                       lnet_ni_unlock(ni);
-                       break;
-               }
+               lnet_ni_lock(ni);
+               lnet_fill_ni_info(ni, config);
+               lnet_ni_unlock(ni);
+               rc = 0;
+               break;
        }
 
        lnet_net_unlock(cpt);
@@ -1944,8 +1947,8 @@ LNetCtl(unsigned int cmd, void *arg)
        lnet_ni_t                *ni;
        int                       rc;
 
-       CLASSERT(LIBCFS_IOC_DATA_MAX >= sizeof(struct lnet_ioctl_net_config) +
-                                       sizeof(struct lnet_ioctl_config_data));
+       BUILD_BUG_ON(sizeof(struct lnet_ioctl_net_config) +
+                    sizeof(struct lnet_ioctl_config_data) > LIBCFS_IOC_DATA_MAX);
 
        switch (cmd) {
        case IOC_LIBCFS_GET_NI:

diff --git a/lnet/lnet/module.c b/lnet/lnet/module.c
index a8eaac5..c080eb4 100644 (file)
--- a/lnet/lnet/module.c
+++ b/lnet/lnet/module.c
@@ -97,12 +97,18 @@ lnet_dyn_configure(struct libcfs_ioctl_hdr *hdr)
          (struct lnet_ioctl_config_data *)hdr;
        int                           rc;
 
+       if (conf->cfg_hdr.ioc_len < sizeof(*conf))
+               return -EINVAL;
+
        mutex_lock(&lnet_config_mutex);
-       if (the_lnet.ln_niinit_self)
-               rc = lnet_dyn_add_ni(LNET_PID_LUSTRE, conf);
-       else
+       if (!the_lnet.ln_niinit_self) {
                rc = -EINVAL;
+               goto out_unlock;
+       }
+       rc = lnet_dyn_add_ni(LNET_PID_LUSTRE, conf);
+out_unlock:
        mutex_unlock(&lnet_config_mutex);
+
        return rc;
 }
 
@@ -113,11 +119,16 @@ lnet_dyn_unconfigure(struct libcfs_ioctl_hdr *hdr)
          (struct lnet_ioctl_config_data *) hdr;
        int                           rc;
 
+       if (conf->cfg_hdr.ioc_len < sizeof(*conf))
+               return -EINVAL;
+
        mutex_lock(&lnet_config_mutex);
-       if (the_lnet.ln_niinit_self)
-               rc = lnet_dyn_del_ni(conf->cfg_net);
-       else
+       if (!the_lnet.ln_niinit_self) {
                rc = -EINVAL;
+               goto out_unlock;
+       }
+       rc = lnet_dyn_del_ni(conf->cfg_net);
+out_unlock:
        mutex_unlock(&lnet_config_mutex);
 
        return rc;
@@ -132,6 +143,10 @@ lnet_ioctl(unsigned int cmd, struct libcfs_ioctl_hdr *hdr)
        case IOC_LIBCFS_CONFIGURE: {
                struct libcfs_ioctl_data *data =
                  (struct libcfs_ioctl_data *)hdr;
+
+               if (data->ioc_hdr.ioc_len < sizeof(*data))
+                       return -EINVAL;
+
                the_lnet.ln_nis_from_mod_params = data->ioc_flags;
                return lnet_configure(NULL);
        }

diff --git a/lnet/lnet/peer.c b/lnet/lnet/peer.c
index b4a075d..523d5b3 100644 (file)
--- a/lnet/lnet/peer.c
+++ b/lnet/lnet/peer.c
@@ -405,7 +405,7 @@ int lnet_get_peer_info(__u32 peer_index, __u64 *nid,
        /* if the cpt number to be examined is >= the number of cpts in
         * the system then indicate that there are no more cpts to examin
         */
-       if (*cpt_iter > lncpt)
+       if (*cpt_iter >= lncpt)
                return -ENOENT;
 
        /* get the current table */

diff --git a/lnet/lnet/router.c b/lnet/lnet/router.c
index 1f31194..3ae2ba3 100644 (file)
--- a/lnet/lnet/router.c
+++ b/lnet/lnet/router.c
@@ -551,7 +551,7 @@ lnet_destroy_routes (void)
 
 int lnet_get_rtr_pool_cfg(int idx, struct lnet_ioctl_pool_cfg *pool_cfg)
 {
-       int i, rc = -ENOENT, lidx, j;
+       int i, rc = -ENOENT, j;
 
        if (the_lnet.ln_rtrpools == NULL)
                return rc;
@@ -560,20 +560,16 @@ int lnet_get_rtr_pool_cfg(int idx, struct lnet_ioctl_pool_cfg *pool_cfg)
                lnet_rtrbufpool_t *rbp;
 
                lnet_net_lock(LNET_LOCK_EX);
-               lidx = idx;
                cfs_percpt_for_each(rbp, j, the_lnet.ln_rtrpools) {
-                       if (lidx-- == 0) {
-                               rc = 0;
-                               pool_cfg->pl_pools[i].pl_npages =
-                                       rbp[i].rbp_npages;
-                               pool_cfg->pl_pools[i].pl_nbuffers =
-                                       rbp[i].rbp_nbuffers;
-                               pool_cfg->pl_pools[i].pl_credits =
-                                       rbp[i].rbp_credits;
-                               pool_cfg->pl_pools[i].pl_mincredits =
-                                       rbp[i].rbp_mincredits;
-                               break;
-                       }
+                       if (i++ != idx)
+                               continue;
+
+                       pool_cfg->pl_pools[i].pl_npages = rbp[i].rbp_npages;
+                       pool_cfg->pl_pools[i].pl_nbuffers = rbp[i].rbp_nbuffers;
+                       pool_cfg->pl_pools[i].pl_credits = rbp[i].rbp_credits;
+                       pool_cfg->pl_pools[i].pl_mincredits = rbp[i].rbp_mincredits;
+                       rc = 0;
+                       break;
                }
                lnet_net_unlock(LNET_LOCK_EX);
        }