* [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
@ 2016-11-06 17:11 Christophe JAILLET
2016-11-06 17:26 ` Oleg Drokin
0 siblings, 1 reply; 6+ messages in thread
From: Christophe JAILLET @ 2016-11-06 17:11 UTC (permalink / raw)
To: oleg.drokin, andreas.dilger, jsimmons, gregkh, emoly.liu
Cc: lustre-devel, devel, linux-kernel, kernel-janitors, Christophe JAILLET
'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.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
2016-11-06 17:11 [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new Christophe JAILLET
@ 2016-11-06 17:26 ` Oleg Drokin
2016-11-07 4:10 ` James Simmons
2016-11-07 21:33 ` Dilger, Andreas
0 siblings, 2 replies; 6+ messages in thread
From: Oleg Drokin @ 2016-11-06 17:26 UTC (permalink / raw)
To: Christophe JAILLET
Cc: andreas.dilger, jsimmons, gregkh, emoly.liu, lustre-devel, devel,
linux-kernel, kernel-janitors
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).
Thanks!
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
2016-11-06 17:26 ` Oleg Drokin
@ 2016-11-07 4:10 ` James Simmons
2016-11-07 10:40 ` Greg KH
2016-11-07 21:33 ` Dilger, Andreas
1 sibling, 1 reply; 6+ messages in thread
From: James Simmons @ 2016-11-07 4:10 UTC (permalink / raw)
To: Oleg Drokin
Cc: Christophe JAILLET, andreas.dilger, gregkh, emoly.liu,
lustre-devel, devel, linux-kernel, kernel-janitors
> 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).
The header lustre_cfg.h is meant to be a UAPI header file. It is used for
our userland tools but with the current shape of lustre_cfg.h upstream our
tools will not build with it. So having kzalloc and kfree in this header
is incorrect. To do this right I need to update our user land tools as
well so we should hold off on these patches.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
2016-11-07 4:10 ` James Simmons
@ 2016-11-07 10:40 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2016-11-07 10:40 UTC (permalink / raw)
To: James Simmons
Cc: Oleg Drokin, devel, andreas.dilger, kernel-janitors,
linux-kernel, Christophe JAILLET, emoly.liu, lustre-devel
On Mon, Nov 07, 2016 at 04:10:16AM +0000, James Simmons wrote:
>
> > 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).
>
> The header lustre_cfg.h is meant to be a UAPI header file. It is used for
> our userland tools but with the current shape of lustre_cfg.h upstream our
> tools will not build with it. So having kzalloc and kfree in this header
> is incorrect. To do this right I need to update our user land tools as
> well so we should hold off on these patches.
Ok, but the code as-is today is incorrect, so that should get fixed
somehow, soon...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
2016-11-06 17:26 ` Oleg Drokin
2016-11-07 4:10 ` James Simmons
@ 2016-11-07 21:33 ` Dilger, Andreas
2016-11-07 22:46 ` Oleg Drokin
1 sibling, 1 reply; 6+ messages in thread
From: Dilger, Andreas @ 2016-11-07 21:33 UTC (permalink / raw)
To: Christophe JAILLET
Cc: James Simmons, Drokin, Oleg, Greg Kroah-Hartman, Liu, Emoly,
Lustre Development List, devel, Linux Kernel Mailing List,
kernel-janitors
On Nov 6, 2016, at 10:26, Drokin, Oleg <oleg.drokin@intel.com> 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 <christophe.jaillet@wanadoo.fr>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new
2016-11-07 21:33 ` Dilger, Andreas
@ 2016-11-07 22:46 ` Oleg Drokin
0 siblings, 0 replies; 6+ messages in thread
From: Oleg Drokin @ 2016-11-07 22:46 UTC (permalink / raw)
To: Dilger, Andreas
Cc: Christophe JAILLET, James Simmons, Greg Kroah-Hartman, Liu,
Emoly, Lustre Development List, devel, Linux Kernel Mailing List,
kernel-janitors
On Nov 7, 2016, at 4:33 PM, Dilger, Andreas wrote:
> On Nov 6, 2016, at 10:26, Drokin, Oleg <oleg.drokin@intel.com> 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.
This patch was actually doing it.
>
> Cheers, Andreas
>
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>> 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-07 22:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-06 17:11 [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new Christophe JAILLET
2016-11-06 17:26 ` Oleg Drokin
2016-11-07 4:10 ` James Simmons
2016-11-07 10:40 ` Greg KH
2016-11-07 21:33 ` Dilger, Andreas
2016-11-07 22:46 ` Oleg Drokin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).