Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757661Ab3GRJIk (ORCPT ); Thu, 18 Jul 2013 05:08:40 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:52026 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756609Ab3GRJIh (ORCPT ); Thu, 18 Jul 2013 05:08:37 -0400 Date: Thu, 18 Jul 2013 10:08:35 +0100 From: Al Viro To: linux-kernel@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, Nathan Rutman Subject: [lustre mess] is mgc_fs_setup() reachable at all? Message-ID: <20130718090835.GZ4165@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8088 Lines: 152 [nathan cc'd since he's the only person mentioned in mgc_request.c] One general observation: drivers/taging/lustre is highly reviewer-hostile... Found by unrelated grep: drivers/staging/lustre/lustre/mgc/mgc_request.c:1040: if (vallen != sizeof(struct super_block)) Huh? The code around that line looks so: 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); } RETURN(rc); } What is going on here? We cast something to struct super_block *? Where does it come from? The function it's in is int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp, obd_count keylen, void *key, obd_count vallen, void *val, struct ptlrpc_request_set *set) which says damn little, other than "multiplexor with no type safety". Who calls that? ; git grep -n -w mgc_set_info_async drivers/staging/lustre/lustre/mgc/mgc_request.c:1001:int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp, drivers/staging/lustre/lustre/mgc/mgc_request.c:1836: .o_set_info_async = mgc_set_info_async, ; Umm... OK, looks like it's only called as a method. Unfortunately, grep for o_set_info_async brings a lot of instances and no callers. After a lot of cursing, the following antisocial Fine Piece Of Code had been located: #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op along with rc = OBP(exp->exp_obd, set_info_async)(env, exp, keylen, key, vallen, val, set); As far as I'm concerned, macros like that are equivalent to spitting into reviewers' eyes. Sometimes out of malice, sometimes just because the authors felt like it would be a cute prank, either way it makes life really unpleasant - when you need to guess which part of method/function name is to be searched for to locate the call sites... it gets old very soon. Anyway, this thing is in static inline int obd_set_info_async(const struct lu_env *env, struct obd_export *exp, obd_count keylen, void *key, obd_count vallen, void *val, struct ptlrpc_request_set *set) Again, the type safety is inexistent, but let's cross the fingers and look for callers (and hope they hadn't pulled a similar cute trick with ## here) ; git grep -n -w obd_set_info_async drivers/staging/lustre/lustre/include/obd_class.h:508:static inline int obd_set_info_async(const struct lu_env *env, drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c:522: rc = obd_set_info_async(req->rq_svc_thread->t_env, drivers/staging/lustre/lustre/llite/dir.c:658: rc = obd_set_info_async(NULL, mgc, sizeof(KEY_SET_INFO), KEY_SET_INFO, drivers/staging/lustre/lustre/llite/llite_lib.c:558: err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM), drivers/staging/lustre/lustre/llite/llite_lib.c:563: err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CACHE_SET), drivers/staging/lustre/lustre/llite/llite_lib.c:1964: obd_set_info_async(NULL, sbi->ll_md_exp, drivers/staging/lustre/lustre/llite/llite_lib.c:1967: obd_set_info_async(NULL, sbi->ll_dt_exp, drivers/staging/lustre/lustre/llite/llite_lib.c:2032: err = obd_set_info_async(NULL, sbi->ll_md_exp, drivers/staging/lustre/lustre/llite/lproc_llite.c:437: rc = obd_set_info_async(NULL, sbi->ll_dt_exp, drivers/staging/lustre/lustre/llite/lproc_llite.c:485: rc = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM), drivers/staging/lustre/lustre/lmv/lmv_obd.c:281: obd_set_info_async(NULL, tgt->ltd_exp, sizeof(KEY_INTERMDS), drivers/staging/lustre/lustre/lmv/lmv_obd.c:2239: err = obd_set_info_async(env, tgt->ltd_exp, drivers/staging/lustre/lustre/lov/lov_obd.c:638: rc = obd_set_info_async(NULL, tgt->ltd_exp, drivers/staging/lustre/lustre/lov/lov_obd.c:2629: err = obd_set_info_async(env, tgt->ltd_exp, drivers/staging/lustre/lustre/lov/lov_obd.c:2633: err = obd_set_info_async(env, tgt->ltd_exp, drivers/staging/lustre/lustre/lov/lov_obd.c:2646: err = obd_set_info_async(env, tgt->ltd_exp, keylen, drivers/staging/lustre/lustre/lov/lov_obd.c:2655: err = obd_set_info_async(env, tgt->ltd_exp, drivers/staging/lustre/lustre/mdc/mdc_request.c:1767: rc = obd_set_info_async(NULL, exp, strlen(KEY_CHANGELOG_CLEAR), drivers/staging/lustre/lustre/obdclass/genops.c:624: rc2 = obd_set_info_async(NULL, obd->obd_self_export, drivers/staging/lustre/lustre/obdclass/obd_mount.c:277: rc = obd_set_info_async(NULL, obd->obd_self_export, drivers/staging/lustre/lustre/obdclass/obd_mount.c:326: rc = obd_set_info_async(NULL, obd->obd_self_export, drivers/staging/lustre/lustre/obdclass/obd_mount.c:425: rc = obd_set_info_async(NULL, obd->obd_self_export, drivers/staging/lustre/lustre/obdclass/obd_mount.c:437: rc = obd_set_info_async(NULL, obd->obd_self_export, ; Oh, joy... 23 call sites to sift through. Which ones are we interested in? The test down in the FPOS that has started it all is also reviewer-hostile - KEY_IS(KEY_SET_FS) has no visible references to function arguments. Oh, well... #define KEY_IS(str) \ (keylen >= (sizeof(str)-1) && memcmp(key, str, (sizeof(str)-1)) == 0) #define KEY_SET_FS "set_fs" and KEY_SET_FS is _NOT_ mentioned anywhere outside that check. Neither is "set_fs" as explicit string constant. I would've assumed the whole thing to be dead code, but... what with the preprocessor tricks we'd already seen there, I wouldn't bet against that thing coming from string concat. Or from macro stringifying set_fs (sans double quotes). Or from any number of sick preprocessor tricks. Oh, well - at least we can go through that list and drop the call sites that pass a different string constant. A few minutes and curses later we are down to these 5: err = obd_set_info_async(env, tgt->ltd_exp, keylen, key, vallen, val, set); in lmv_set_info_async(), and err = obd_set_info_async(env, tgt->ltd_exp, keylen, key, sizeof(int), &mgi->group, set); err = obd_set_info_async(env, tgt->ltd_exp, keylen, key, vallen, ((struct obd_id_info*)val)->data, set); err = obd_set_info_async(env, tgt->ltd_exp, keylen, key, sizeof(*info->capa), info->capa, set); err = obd_set_info_async(env, tgt->ltd_exp, keylen, key, vallen, val, set); in lov_set_info_async(). ; git grep -n -w lmv_set_info_async drivers/staging/lustre/lustre/lmv/lmv_obd.c:2212:int lmv_set_info_async(const struct lu_env *env, struct obd_export *exp, drivers/staging/lustre/lustre/lmv/lmv_obd.c:2661: .o_set_info_async = lmv_set_info_async, ; IOW, this one is another o_set_info_async instance and key/keylen/val/vallen is passed from its caller as-is. ; git grep -n -w lov_set_info_async drivers/staging/lustre/lustre/lov/lov_obd.c:2559:static int lov_set_info_async(const struct lu_env *env, struct obd_export *exp, drivers/staging/lustre/lustre/lov/lov_obd.c:2850: .o_set_info_async = lov_set_info_async, ; ... and this one is also o_set_info_async instance. In one of 4 call sites we appear to pass the arguments as-is, the rest... key/keylen is passed as-is, val/vallen isn't. In any case, key must have originated in one of the call sites we'd already eliminated. So unless I've missed something in that paragon of good taste, there is no call chain that could've lead to mgc_set_info_async() with key being "set_fs". Incidentally, there is a couple of recursive loops in there that might or might not lead to stack overflows. Could somebody familiar with that thing confirm that this is, in fact, dead code? As the matter of fact, could maintainers please stand up and put their names into MAINTAINERS? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/