Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752046AbcKGVeU convert rfc822-to-8bit (ORCPT ); Mon, 7 Nov 2016 16:34:20 -0500 Received: from mga14.intel.com ([192.55.52.115]:10861 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750981AbcKGVd3 (ORCPT ); Mon, 7 Nov 2016 16:33:29 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,459,1473145200"; d="scan'208";a="1065247989" From: "Dilger, Andreas" To: Christophe JAILLET CC: James Simmons , "Drokin, Oleg" , Greg Kroah-Hartman , "Liu, Emoly" , Lustre Development List , "devel@driverdev.osuosl.org" , Linux Kernel Mailing List , "kernel-janitors@vger.kernel.org" Subject: Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new Thread-Topic: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new Thread-Index: AQHSOFDRzFOIEFiiPUi5pE4DKwULLKDMu08AgAHXTgA= Date: Mon, 7 Nov 2016 21:33:26 +0000 Message-ID: References: <20161106171123.24929-1-christophe.jaillet@wanadoo.fr> <97207CF4-21C1-4351-92BF-6F28E7281CF6@intel.com> In-Reply-To: <97207CF4-21C1-4351-92BF-6F28E7281CF6@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.255.92.27] Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3174 Lines: 87 On Nov 6, 2016, at 10:26, Drokin, Oleg wrote: > > Hello! > > On Nov 6, 2016, at 12:11 PM, Christophe JAILLET wrote: > >> 'lustre_cfg_new()' can return ERR_PTR(-ENOMEM). >> Handle these errors and propagate the error code to the callers. >> >> Error handling has been rearranged in 'lustre_process_log()' with the >> addition of a label in order to free some resources. > > I wonder if we should just make it return NULL on allocation failure, > and then at least the other error handling that is there (i.e. in your other patch) > would become correct. > This would make handling in mgc_apply_recover_logs incorrect, but it's already > geared towards this sort of handling anyway, as it discards the passed error > and sets ENOMEM unconditionally (just need to revert 3092c34a in a way). I'd agree with Oleg that returning NULL is the preferable solution here. There are also callers of lustre_cfg_new() in class_config_llog_handler(), do_lcfg(), and lustre_end_log() that do not check error returns at all that should be fixed at the same time. Cheers, Andreas >> >> Signed-off-by: Christophe JAILLET >> --- >> drivers/staging/lustre/lustre/obdclass/obd_mount.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/obd_mount.c b/drivers/staging/lustre/lustre/obdclass/obd_mount.c >> index 59fbc29aae94..5473615cd338 100644 >> --- a/drivers/staging/lustre/lustre/obdclass/obd_mount.c >> +++ b/drivers/staging/lustre/lustre/obdclass/obd_mount.c >> @@ -89,11 +89,14 @@ int lustre_process_log(struct super_block *sb, char *logname, >> lustre_cfg_bufs_set(bufs, 2, cfg, sizeof(*cfg)); >> lustre_cfg_bufs_set(bufs, 3, &sb, sizeof(sb)); >> lcfg = lustre_cfg_new(LCFG_LOG_START, bufs); >> + if (IS_ERR(lcfg)) { >> + rc = PTR_ERR(lcfg); >> + goto out_free; >> + } >> + >> rc = obd_process_config(mgc, sizeof(*lcfg), lcfg); >> lustre_cfg_free(lcfg); >> >> - kfree(bufs); >> - >> if (rc == -EINVAL) >> LCONSOLE_ERROR_MSG(0x15b, "%s: The configuration from log '%s' failed from the MGS (%d). Make sure this client and the MGS are running compatible versions of Lustre.\n", >> mgc->obd_name, logname, rc); >> @@ -104,6 +107,9 @@ int lustre_process_log(struct super_block *sb, char *logname, >> rc); >> >> /* class_obd_list(); */ >> + >> +out_free: >> + kfree(bufs); >> return rc; >> } >> EXPORT_SYMBOL(lustre_process_log); >> @@ -127,6 +133,9 @@ int lustre_end_log(struct super_block *sb, char *logname, >> if (cfg) >> lustre_cfg_bufs_set(&bufs, 2, cfg, sizeof(*cfg)); >> lcfg = lustre_cfg_new(LCFG_LOG_END, &bufs); >> + if (IS_ERR(lcfg)) >> + return PTR_ERR(lcfg); >> + >> rc = obd_process_config(mgc, sizeof(*lcfg), lcfg); >> lustre_cfg_free(lcfg); >> return rc; >> @@ -159,6 +168,9 @@ static int do_lcfg(char *cfgname, lnet_nid_t nid, int cmd, >> lustre_cfg_bufs_set_string(&bufs, 4, s4); >> >> lcfg = lustre_cfg_new(cmd, &bufs); >> + if (IS_ERR(lcfg)) >> + return PTR_ERR(lcfg); >> + >> lcfg->lcfg_nid = nid; >> rc = class_process_config(lcfg); >> lustre_cfg_free(lcfg); >> -- >> 2.9.3 >