Messages in this thread | | | Date | Tue, 26 Nov 2013 19:33:00 +0100 | From | Levente Kurusa <> | Subject | Re: [PATCH v2 02/12] staging/lustre/llog: MGC to use OSD API for backup logs |
| |
Hi,
just a few comments, mostly code style.
2013-11-26 16:04, Peng Tao: > @@ -1011,23 +1083,17 @@ int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp, > } > if (KEY_IS(KEY_SET_FS)) { > struct super_block *sb = (struct super_block *)val; > - struct lustre_sb_info *lsi; > + > if (vallen != sizeof(struct super_block)) > return -EINVAL; > - lsi = s2lsi(sb); > - rc = mgc_fs_setup(exp->exp_obd, sb, lsi->lsi_srv_mnt); > - if (rc) { > - CERROR("set_fs got %d\n", rc); > - } > + > + rc = mgc_fs_setup(exp->exp_obd, sb); > return rc;
Why set rc when you return it on the next statement? Wouldn't 'return mgc_fs_setup(exp->exp_obd, sb);' be better?
> } > if (KEY_IS(KEY_CLEAR_FS)) { > if (vallen != 0) > return -EINVAL; > rc = mgc_fs_cleanup(exp->exp_obd); > - if (rc) { > - CERROR("clear_fs got %d\n", rc); > - } > return rc;
Same here possibly.
> + /* Copy the setup log locally if we can. Don't mess around if we're > + * running an MGS though (logs are already local). */ > + if (lctxt && lsi && IS_SERVER(lsi) && !IS_MGS(lsi) && > + cli->cl_mgc_configs_dir != NULL && > + lu2dt_dev(cli->cl_mgc_configs_dir->do_lu.lo_dev) == > + lsi->lsi_dt_dev) {
I think that is *really* ugly! (The last comparison is separated into two lines)
-- Regards, Levente Kurusa
| |